refactor: migrate to pure es modules #2553
Conversation
|
Very exciting to see this! |
|
The work is done, but maybe this is too big to review... 👀 |
saschanaz
force-pushed
the
async-import
branch
from
7671b40 to
8c0aa29
Compare
| @@ -0,0 +1,22 @@ | |||
| // @ts-check | |||
| // Temporary workaround until browsers get real import-maps | |||
There was a problem hiding this comment.
So much sadness... but ok :)
There was a problem hiding this comment.
What's going to be the long term strategy so we don't need this?
There was a problem hiding this comment.
Official import maps implementation can replace this with traditional import x as "module".
| copyButton.classList.add("respec-button-copy-paste", "removeOnSave"); | ||
| const copyButtonPromise = createButton(); | ||
|
|
||
| async function loadSVG() { |
There was a problem hiding this comment.
I wonder if we should generalize this into a util function... loadTextAsset() or something. I imagine we eventually want to kill these methods once we address whatever is requiring them.
There was a problem hiding this comment.
That's essentially https://github.com/w3c/respec/pull/2553#discussion_r339940372, and we can't remove it as I replied there:
No, dynamic import with dynamic value will prevent bundler from merging assets into one file, because they are not smart enough 😂
| export function run() { | ||
| const cssPromise = loadStyle(); | ||
|
|
||
| async function loadStyle() { |
There was a problem hiding this comment.
as above, loadTextAsset()?
| new Blob([workerScript], { type: "application/javascript" }) | ||
| ); | ||
| export const worker = new Worker(workerURL); | ||
| async function loadWorkerScript() { |
There was a problem hiding this comment.
And same here... loadTextAsset(asset, path) or something.
There was a problem hiding this comment.
Pretty cool! import-maps thing is kinda sad, but 🤷🏽♂️... most important thing for me is that the bundles are debuggable and get automatically recompiled, the same way that webpack works. I imagine it must be the same. @saschanaz, what's the magic to get that to happen? I'd love to quickly try this out.
|
Ok, looks like no dev server :( but that might be ok. rollup/rollup#2385 (comment) We might be able to work around it somehow. There are a few options. |
Have we used webpack dev server? 👀 |
Not in this project, no (we use |
| "release": "node ./tools/release.js", | ||
| "server": "npm start", | ||
| "snyk-protect": "snyk protect", | ||
| "snyk": "snyk", | ||
| "start": "npm run build:components && serve", | ||
| "start": "serve", |
There was a problem hiding this comment.
This is where the "dev server" would come in... basically, what we want here is for rollup to be watching files and automatically running babel for us.
There was a problem hiding this comment.
oh, and also automatically building the "development" bundle.
There was a problem hiding this comment.
There was a problem hiding this comment.
We don't need babel anymore here, so you just want to refresh your browser and that's all. (Only if you are using non-built version)
Everything should be same as before, you build the bundle and insert breakpoints 👍 |
|
Ok, no babel is awesome - so that’s fantastic. But the rebuilding step is no good as it makes testing in Karma super cumbersome. That’s the last critical thing I’d really like to address. It seems it’s doable with the watch thing above. |
Not sure what's the rebuilding step here. If you mean |
|
Slightly concerned about the bundle size increase. curl https://www.w3.org/Tools/respec/respec-w3c | wc -c
# 328923
$ curl https://deploy-preview-2553--respec-pr.netlify.com/respec-w3c.js | wc -c
# 389695
# +60722 bytes addedWith gzip: $ curl -H 'Accept-encoding: gzip' https://www.w3.org/Tools/respec/respec-w3c | wc -c
# 94909
$ curl -H 'Accept-encoding: gzip' https://deploy-preview-2553--respec-pr.netlify.com/respec-w3c.js | wc -c
# 121864
# +26955 bytes added |
Oops, the jquery commit did something wrong. It actually decreases without it. |
|
Happy with the 28043 byte reduction in size (3481 bytes in gzip) 🎉 |
|
@saschanaz We still need to |
Right. |
|
Right, so ideally we should never need to run "build:w3c" or the other builders... the watcher should be doing that automatically for us. |
|
That sound great, and then it sounds like an extra feature. Maybe it can be implemented after this PR? |
|
Yes, definitely could be an extra feature (and new pull request)... but what need need to confirm is that it's possible (sounds like it is, given the watcher thing) - otherwise, we will need to later switch to React. |
|
Lol, not react! webpack. |
|
Seems there are also third-party rollup dev server libraries, so I think it should be possible to implement. |
|
Migration to React will be even more difficult than to nanohtml 😂 |
* develop: v24.34.2 fix(tools/release): components no longer built chore: don't update deps chore(package): npm audit fixes Update eslint-plugin-jasmine to the latest version 🚀 (#2571) chore(package): update eslint-plugin-jasmine to version 3.0.0 chore(package): update deps (#2562) fix(core/dfn-finder): explicit empty dfn-for on top level things (#2558) refactor: migrate to pure es modules (#2553) fix(tools/copydeps): gracely ignore when no folder (#2556) refactor: force extensions for imports (#2555)
Finally some part of #2187 😄
Closes #1975