Reuse webpack-dev-server when servlet-container reloads #5987
Conversation
project-bot
bot
added this to Review in progress
in OLD Vaadin Flow ongoing work (Vaadin 10+)
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
from
cf3be43
to
4efed9c
Compare
…erver. - The regression was introduced in #5852 when removing the system property informing about the parameters of a webpack-dev-server already running. - The solution in this commit is to write a temporary file in `tmp.dir` containing the info of the running webpack-dev-server. - The file name is computed based on the JVM identifier and the thread group which is constant for the containers used in development (tc, jetty, spring-boot) - Having an unique name for the file, allows to run multiple projects in the same machine - The file is removed when the servlet container is stoped. - There might be an issue, in the case we want to start two servlet-containers in the same JVM, but that is a very corner case.
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
2 times, most recently
from
46646c4
to
47435ef
Compare
|
Should the "fixes" link point to #5938 instead? |
There was a problem hiding this comment.
sure, changed in description
Reviewable status: all discussions resolved, 0 of 1 LGTMs obtained
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
from
47435ef
to
c62f4d4
Compare
There was a problem hiding this comment.
Reviewed 1 of 5 files at r3.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
new StopDevMode()
This has been introduced as a fix for #5809.
Removing this fixes one thing but introduces another regression (in the same way as introducing StopDevMode class).
So we are playing in ping-pong here.
Silent removal of this code without alternative suggestion is not an option.
We need another way to take care about running webpack on restart....
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
new StopDevMode()This has been introduced as a fix for #5809.
Removing this fixes one thing but introduces another regression (in the same way as introducing
StopDevModeclass).
So we are playing in ping-pong here.Silent removal of this code without alternative suggestion is not an option.
We need another way to take care about running webpack on restart....
Right
- First, I’m monitoring the TC build so as this not causes problems in TC by the moment
- Second, I will prepare a PR with any of the fixes I suggest in my commit message:
- Do not run tests in dev-mode (we shouldn’t encourage this for our users because tests do not modifies classes dynamically)
- In the case we need some IT for specifically test DevModeHandler feature, we should use jetty in fork mode
- In addition we might modify maven plugin or whatever to pass a parameter for killing webpack on server destroy, but I think we should avoid this
There was a problem hiding this comment.
Reviewed 2 of 6 files at r1, 3 of 5 files at r3.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Are you saying that #5809 is invalid ?
First, I’m monitoring the TC build so as this not causes problems in TC by the moment
Doesn't prove anything. Even if it doesn't add any issues at the moment it doesn't mean we won't have them in the future.
Do not run tests in dev-mode
We need those tests. And we are running them. So I would say this is not an option as well.
Do you see any other option to kill webpack and avoiding issues with restart ?
First of all: why the started webpack is not reused at all (but a new one started) as #5809 says ?
Isn't it the same case as for "restart" ?
Can we "schedule" webpack server killing somehow ?
The simplest way would be to start a new thread which kills the webpack after some delay in case there are no new active servlet context. So the job is cancelled once a new servlet context is initialized.
Or some other way ?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Doesn't prove anything. Even if it doesn't add any issues at the moment it doesn't mean we won't have them in the future.
No, what I meant is that this PR is green, it can be merged, and work separately in another, so as this PR is focused in the ticket it has to fix.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
Doesn't prove anything. Even if it doesn't add any issues at the moment it doesn't mean we won't have them in the future.
No, what I meant is that this PR is green, it can be merged, and work separately in another, so as this PR is focused in the ticket it has to fix.
That PR introduces a known regression.
So I don't think this is acceptable way.
I would may be go at the moment with an additional "hidden" deployment parameter which
enables back StopDevMode in its way and stop the server only if this parameter is set to true .
Then we should modify all our test servlets (as a part of this PR) to set this parameter to true.
In case of other improvement we may do them as a separate PR.
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
We need those tests. And we are running them. So I would say this is not an option as well.
We need tests for npm-mode, it does not mean that those npm tests need to be run against in memory webpack-dev-server bundle, but against the bundle in file system produced by webpack because the result for the test checking an application feature is exactly the same.
I want to stress, that only for the case of ‘flow’ project and not for any other application, we would need to test the DevModeHandler stack, so we need a IT for that, but it not means that we should run all test suite tests
Do you see any other option to kill webpack and avoiding issues with restart ?
Right now webpack is killed when JVM exits, there in no other way, since servlet container does not gives a way that informs that it exits. Hence what I’m proposing is to use fork mode in jetty, though I need to verify
First of all: why the started webpack is not reused at all (but a new one started) as #5809 says ?
Because 1.- servlet container stops all servlet and context listeners in each hot-reload 2.- vaadin context is restarted in each hot-reload so port is not kept in memory
Isn't it the same case as for "restart" ?
Can we "schedule" webpack server killing somehow ?
only in the JVM exit hook (at least I didn’t found any other way)
The simplest way would be to start a new thread which kills the webpack after some delay in case there are no new active servlet context. So the job is cancelled once a new servlet context is initialized.
Or some other way ?
We lose the reference to the thread, and the thread does not know anything about the container
…emon for each re-deploy. - The regresion was introduced in #5827 when introducing a StopDevMode listener that stops webpack whenenver the servlet container reloads - If we want to stop daemon for testing purpouses the solution is to pass a parameter from the test build or better run jetty in fork mode. - Actually it makes no sense to run tests with webpack-dev-server but webpack since java code does not change during the tests
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
from
c62f4d4
to
fce178f
Compare
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Because 1.- servlet container stops all servlet and context listeners in each hot-reload 2.- vaadin context is restarted in each hot-reload so port is not kept in memory
Sorry, I didn't get on which question you are answering.
#5809 is not about hot reload.
So why both 1 and 2 are not applicable as an answer to the question.
We lose the reference to the thread, and the thread does not know anything about the container
Yes, and I don't see a problem since we have a reference to the thread.
The listener will be the same. Every listener know about the one single thread which is going to kill the web pack.
Once context listener is called with the context initialized it lets the thread know that the task should be cancelled.
I don't see any problem here at all.
The only question is whether scheduling is good enough. Which depends on answer to the previous question.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
#5809 is not about hot reload.
Is related to testing, so fixing testing should not break hot reload
Yes, and I don't see a problem since we have a reference to the thread.
Can we prototype a different design, and do in separated PR? this tries to behaves in almost the same way that it was working for the beta period
In summary, I propose to fix #5809 in a separated PR, so I’ll open a ticket for that discussion and design
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
#5809 is not about hot reload.
Is related to testing, so fixing testing should not break hot reload
The question wasn't about hot reload. It's absolutely obvious what happens with hot reload.
The question was:
why the started webpack is not reused at all (but a new one started) as #5809 says ?
You have made #5809 . Why webpack is not reused at all as it's described in #5809 ? (it's not about hot reload, it's only about #5809). What is the exact issue which #5809 address ?
Can we prototype a different design, and do in separated PR? this tries to behaves in almost the same way that it was working for the beta period
We can do anything as long as this PR doesn't break other things.
But it does.
So either :
- let's agree that Make
npm-dev-modetests work in CI #5809 is invalid - let's find a quick fix which allows to keep stop dev mode not only on JVM exit.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @denis-anisimov)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
#5809 is not about hot reload.
Is related to testing, so fixing testing should not break hot reload
The question wasn't about hot reload. It's absolutely obvious what happens with hot reload.
The question was:why the started webpack is not reused at all (but a new one started) as #5809 says ?
You have made #5809 . Why webpack is not reused at all as it's described in #5809 ? (it's not about hot reload, it's only about #5809). What is the exact issue which #5809 address ?
Can we prototype a different design, and do in separated PR? this tries to behaves in almost the same way that it was working for the beta period
We can do anything as long as this PR doesn't break other things.
But it does.So either :
- let's agree that Make
npm-dev-modetests work in CI #5809 is invalid- let's find a quick fix which allows to keep stop dev mode not only on JVM exit.
The way I see it is that the changes for #5809 made the product broken for users. It works in a basic scenario, but it is broken for any serious usage when using auto reload features.
And #5809 is about our internal testing, so those changes should be reverted ASAP and we should address that in some other way.
As now this is blocking Flow 2.0.2 and 14 RC3 which are required for further internal validation testing, I would just do the revert as suggested in this PR.
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java, line 163 at r1 (raw file):
.addShutdownHook(new Thread(() -> { webpackProcess.destroy(); removeRunningDevServerPort(context);
Personally, I'd prefer that these static methods were reference to as FrontendUtil.whatEverTheMethodIs() just to make it easier to understand, where the method is coming from.
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
int port = getRunningDevServerPort(service.getContext()); if (port > 0) { return DevModeHandler.prepareConnection(port, "/stats.json", "GET").getInputStream();
I am not fond of the two-way relationship between FrontendUtils and DevModeHandler. prepareConnection(...) could just as well live under FrontendUtils as it does not seem particularly specific to the DevModeHandler.
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 516 at r1 (raw file):
private static File computeDevServerPortFileName() { // The thread group is the same in each servlet-container restart
I am unfamiliar with this claim and cannot verify it easily. Do you happen to have some source that could be added here? I am unsure why some web container implementation would not opt to do something volatile here.
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 525 at r1 (raw file):
String uniqueUid = UUID.nameUUIDFromBytes((jvmUniqueName + threadGroup).getBytes()).toString(); // File is placed in the user temporary folder, it works for all platrforms
Typo in "platforms"
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 548 at r1 (raw file):
try { String portString = FileUtils.readFileToString(portFile, "UTF-8").trim(); port = (int)Long.parseLong(portString);
Wouldn't Integer.parseInt(portString); be enough here? If the port is long but not int, somethings wrong already.
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, pleku (Pekka Hyvönen) wrote…
The way I see it is that the changes for #5809 made the product broken for users. It works in a basic scenario, but it is broken for any serious usage when using auto reload features.
And #5809 is about our internal testing, so those changes should be reverted ASAP and we should address that in some other way.As now this is blocking Flow 2.0.2 and 14 RC3 which are required for further internal validation testing, I would just do the revert as suggested in this PR.
We could (at some point, be it now or later) do, what @denis-anisimov suggests and add a "hidden" parameter, which decides where to register the shutdown listener: as a JVM shutdown listener or Context listener.
As I understand, it is only required for us to be able to run dev-mode ITs where the contents keep changing but JVM stays the same, right?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
why the started webpack is not reused at all (but a new one started) as #5809 says ?
No, #5809 is about not leaving running webpack-dev-server daemon in CI, but not about reusing, we dont need to reuse webpack-dev-server in our tests, nor in CI.
So in the case we need some test in npm-dev-mode in CI as the issue says, we need to be sure that it is stoped. The way to stop that is as I have already described
- Use fork mode
- Use some parameter so as DevModeServlet knows that it’s working in a CI and must stop the server on thread destruction
let's agree that #5809 is invalid
It’s not actually invalid, it’s describing a problem in CI, hence we need to do something, but the solution can be in code or in test configuration as said ^
let's find a quick fix which allows to keep stop dev mode not only on JVM exit.
I already tried to find a way to detect when the servlet-container exits, but couldn’t. The only option I found is when the servlet context is destroyed, but that is invalid because breaks hot-reload
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
As I understand, it is only required for us to be able to run dev-mode ITs where the contents keep changing but JVM stays the same, right?
exactly, but the question here is should we run dev-mode in ITs? it made sense in bower because implementation was different, but in npm we have the same bundle that we get with npm package so I would discourage that users use that. If we agree on supporting tests with the webpack daemon running, we should implement what you suggest: a parameter for making DevModeHandler know that it’s running for a test
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, ujoni (Joni) wrote…
I am not fond of the two-way relationship between
FrontendUtilsandDevModeHandler.prepareConnection(...)could just as well live underFrontendUtilsas it does not seem particularly specific to theDevModeHandler.
Also, in theory, these methods could be called from places where the ThreadGroup is not what we'd expect. Should an attempt to ascend the tree via getParent() be made?
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
As I understand, it is only required for us to be able to run dev-mode ITs where the contents keep changing but JVM stays the same, right?
exactly, but the question here is should we run dev-mode in ITs? it made sense in bower because implementation was different, but in npm we have the same bundle that we get with
npm packageso I would discourage that users use that. If we agree on supporting tests with the webpack daemon running, we should implement what you suggest: a parameter for making DevModeHandler know that it’s running for a test
- I give up. But it doesn't mean I agree. I still disagree. You may discard me from the review.
- I didn't find the answer to my question about Make
npm-dev-modetests work in CI #5809 . All "potential answers" was about something else - This "fix" may potentially breaks our tests. Technically yes, it's an internal thing. Practically: I've found a huge number of issues when I started to work on enabling tests. If we are OK that our validation will be broken (doesn't have to be though) which allows to add more bugs/regressions then it's absolutely OK.
Runtime.getRuntime().addShutdownHookis not reliable. JVM may exist abnormally ( and no, this is not an exceptional case, it's quite possible) andaddShutdownHookwon't be called. Which means the webpack won't be killed.
But please take the responsibility about this PR. As I said: I disagree and don't want to take the responsibility for this "fix".
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Runtime.getRuntime().addShutdownHook is not reliable. JVM may exist abnormally ( and no, this is not an exceptional case, it's quite possible) and addShutdownHook won't be called. Which means the webpack won't be killed.
This was a good catch, I was meant to point that out, but somehow forgot.
@denis-anisimov was this you original 5809 question:
First of all: why the started webpack is not reused at all (but a new one started) as #5809 says ?
Does it refer only to test usage or in general?
Also, from what I can tell this discussion boils down to either:
- Maintain the possibility of running all ITs in dev mode as previously
- Or, test dev mode separately and run ITs against production mode
Or have I misunderstood? If correct, it would seems like a discussion for the whole team in terms of "What is acceptable level of testing for us to be fairly confident in our setup".
I am a little bit uncertain where to go from here, as this would be a good PR to get in, but personally I like our current test setup where we are able to run tests in both dev and prod modes for full coverage.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
was this you original 5809 question:
First of all: why the started webpack is not reused at all (but a new one started) as #5809 says ?
was this you original 5809 question:
It is about #5809 all the time. The ticket has poor description.
It just state some issue but has not any explanation.
I don't get from the ticket why we need to stop webpack in the end of test(s) execution.
Two options:
- either JVM exits then why webpack is not killed.
- JVM doesn't exit but it's reused by another Jetty . But then webpack which has been started by the previous Jetty should have been reused. So why do we need to kill it ?
May be some third option ? Which I may not even imagine since two previous covers all the cases.
Also, from what I can tell this discussion boils down to either:
I'm trying to avoid regressions which may be caused by this PR.
At the moment at least there are no problems on CI with extra running webpacks instances.
And we are running dev mode tests.
I want to keep this: it's proven that we need both dev mode tests and production mode tests.
Both are executed: dev mode are executed as a part of PR validation. Production mode are executed on SNAPSHOT trigger ( I think , at least quite soon).
So I it's not important how but we should be able to run dev mode tests.
I'm not confident that this PR won't break at some point or CI because of a lot of running webpacks.
But may be it won't: I don't get answer to my question about #5809.
And anyway: as I said Runtime.getRuntime().addShutdownHook is a bad code from the very beginning. And this PR revert it in the initial bad state.
The overall design of starting/stopping the webpack is not well thought initially.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
And #5809 is about our internal testing, so those changes should be reverted ASAP and we should address that in some other way.
@pleku @denis-anisimov Opened ticket for the testing issue so as we can unblock this PR and continue with the regression fix #5990
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
And #5809 is about our internal testing, so those changes should be reverted ASAP and we should address that in some other way.
@pleku @denis-anisimov Opened ticket for the testing issue so as we can unblock this PR and continue with the regression fix #5990
As I said: I'm not going to unblock this.
You may discard me.
There was a problem hiding this comment.
Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
flow-server/src/main/java/com/vaadin/flow/server/startup/ServletContextListeners.java, line 36 at r1 (raw file):
Previously, denis-anisimov (Denis) wrote…
As I said: I'm not going to unblock this.
You may discard me.
ok, thanks
There was a problem hiding this comment.
Dismissed @denis-anisimov from a discussion.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @manolo)
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @denis-anisimov and @ujoni)
flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java, line 163 at r1 (raw file):
Previously, ujoni (Joni) wrote…
Personally, I'd prefer that these static methods were reference to as
FrontendUtil.whatEverTheMethodIs()just to make it easier to understand, where the method is coming from.
Not static anymore
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, ujoni (Joni) wrote…
Also, in theory, these methods could be called from places where the
ThreadGroupis not what we'd expect. Should an attempt to ascend the tree viagetParent()be made?
- I moved those methods that actually only makes sense in the DevModeHandler to it.
- Now most methods are private, so ThreadGroup is only be used from the DevModeHandler
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 516 at r1 (raw file):
Previously, ujoni (Joni) wrote…
I am unfamiliar with this claim and cannot verify it easily. Do you happen to have some source that could be added here? I am unsure why some web container implementation would not opt to do something volatile here.
I did an empiric debug about which thing I could use as something that it is always the same. Thread Id is not valid, and we cannot go up to Threads, only ThreadGroup is valid to transverse up. ThreadGroup is the same in all servlet container tested because restart happens in this group.
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 525 at r1 (raw file):
Previously, ujoni (Joni) wrote…
Typo in "platforms"
Done.
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 548 at r1 (raw file):
Previously, ujoni (Joni) wrote…
Wouldn't
Integer.parseInt(portString);be enough here? If the port islongbut notint, somethings wrong already.
Done.
There was a problem hiding this comment.
Javadoc thing:
/opt/agent/work/a69251da2b2bb9e9/flow-server/src/main/java/com/vaadin/flow/server/DevModeHandler.java:366: error: @param name not found
* @param port
Reviewed 3 of 3 files at r4.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @manolo)
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
- I moved those methods that actually only makes sense in the DevModeHandler to it.
- Now most methods are private, so ThreadGroup is only be used from the DevModeHandler
Alright. This still seems somewhat iffy. I am not sure how to formulate any sensible test for this, either. This could be in the new ticket about testing, if it is not already
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @ujoni)
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, ujoni (Joni) wrote…
Alright. This still seems somewhat iffy. I am not sure how to formulate any sensible test for this, either. This could be in the new ticket about testing, if it is not already
Added a test for getting the stats file from the proxy, there are tests already for getting the file from the classpath
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
2 times, most recently
from
ef403d8
to
b2bc905
Compare
…tance. Added a test for checking getting the stats file from webpack server
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
from
b2bc905
to
bb47ffa
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, 0 of 1 LGTMs obtained (waiting on @ujoni)
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
Added a test for getting the stats file from the proxy, there are tests already for getting the file from the classpath
Marked DevModeHandler as final, and remove constructor used in tests, so as ppl cannot extend it. In this way the file name cannot be called from a different threadGroup
manolo
force-pushed
the
mcm/npm/fix-webpack-reload
branch
from
55d850f
to
f0f4749
Compare
There was a problem hiding this comment.
Reviewed 5 of 6 files at r5, 1 of 1 files at r6.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @ujoni)
flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendUtils.java, line 355 at r1 (raw file):
Previously, manolo (Manuel Carrasco Moñino) wrote…
Added a test for getting the stats file from the proxy, there are tests already for getting the file from the classpath
Hmm. I dont know how my response is here, instead of the ThreadGroup conversation.. but.. thanks!
There was a problem hiding this comment.
Reviewed 2 of 2 files at r7.
Reviewable status:complete! all discussions resolved, 1 of 1 LGTMs obtained
ujoni
dismissed
denis-anisimov’s stale review
Will hopefully be resolved via #5990
Fixes #5938
This change is