Cleanup client build setup, fix IE<=11 compatibility #1270
Conversation
yyx990803
changed the title
yyx990803
changed the title
There was a problem hiding this comment.
This looks good from what I can see except for the one comment I have. (I could be wrong so please feel free to correct me).
I'll approve once comment addressed and then I'll also have @sokra and @SpaceK33z review because of the importance of this PR. Thanks for all the time and effort that you spent on this Evan.
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "presets": ["env"] | |||
There was a problem hiding this comment.
This would default to CJS module output no? If so, should we be switching this to "modules":false?
There was a problem hiding this comment.
The client source is still using require, so having modules: false has no effect. Or we can convert to use ES modules imports and enable modules: false, but that may break compat with webpack 2 (not sure if that's still a requirement)
|
I really hope that you merge this request. It's not just old desktop browsers (that are very easy to upgrade) but other smart devices that has web apps installed on them. That you cannot upgrade. For instance smart tvs/consoles etc. I ran into this issue some days ago and it took me many days before I found out that it was |
|
We have no objections to the methods here, we are just wanting to make sure
it's reviewed by multiple eyes. I've not spent much time in
webpack-dev-server and want to make sure this is good on all fronts.
Thanks for the patience.
…On Sat, Jan 13, 2018, 10:17 AM Olle Bröms ***@***.***> wrote:
I really hope that you merge this request. It's not just old desktop
browsers (that are very easy to upgrade) but other smart devices that has
web apps installed on them. That you cannot upgrade. For instance smart
tvs/consoles etc.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1270 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADQBMAkdowXiQ-yTkOJq2UZNuJ-ZndwLks5tKPMcgaJpZM4RYpcg>
.
|
There was a problem hiding this comment.
Looking good. I haven't maintained WDS for some time, but I fully agree with the IE<=11 compat and the implementation looks fine.
@TheLarkInn your comment was answered, so I assume you also approve now?
|
Thanks |
|
@sokra looks like dependencies are still not being transpiled (SS taken with Win8 / IE10):
|
For Bugs and Features; did you add new tests?
No, because the bug case is CLI usage only and not testable in the current test setup.
Motivation / Use-Case
Fixes #1268.
#1242 was attempting to fix older browser compatibility by transpiling client bundles with babel. However I didn't notice that the bundles are not used in all cases. It introduced a regression which causes client files to contain ES2015 code when using inline mode via the CLI. This PR fixes that by ensuring the client injected in lib/util/addDevServerEntrypoints.js is the transpiled version as well.
Breaking Changes
None.
Additional Info
Notable Changes
Previously, source files for all 3 clients and their built bundles are mixed in the same directory, making it very confusing. This PR reorganized client files by moving all client source files in to a separate directory,
/client-src, and further separating the three clients into their own directories:/client-src/default/client-src/live/client-src/sockjsIn addition to the bundles, files in
/client-src/defaultare transpiled directly viababel-cliso that they can be bundled directly in inline mode. (seetranspile:indexscript)/clientnow holds only built/transpiled bundles and files, and is added to.gitignore. Built files and bundles have the exact same file layout with pre-PR state of/client, so it should provide complete backwards compat.