-
-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: use parcel for packaging ReSpec #1493
Conversation
(The result should be respec-w3c-common.js rather than basic.js) |
package.json
Outdated
@@ -71,7 +71,7 @@ | |||
"test:karma": "npm run karma", | |||
"prepare": "npm run snyk-protect", | |||
"prepublish": "npm run snyk-protect", | |||
"parcel": "parcel ./examples/basic.html -d ./builds" | |||
"parcel": "parcel ./builds/respec-w3c-common.js -d ./builds/output" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saschanaz Done!
@marcoscaceres Can we have it merged now? |
I’m still unsure what this does? I’m sure it does something useful, but can you help me understand what advantages it brings to the project. |
@marcoscaceres Parcel bundles |
@saschanaz, what’s your opinion? Worth using? |
@@ -77,16 +76,6 @@ See original source for licenses: https://github.com/w3c/respec */ | |||
window.respecVersion = "${version}"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm run parcel
currently skips adding boilerplate, we should add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note that this is also broken in master. (Comments disappear!))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm run parcel currently skips adding boilerplate, we should add it.
Which boilerplate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't really tried this PR yet, but I'm not sure why the test failures we've seen on #1489 do not occur here (as parcel also uses uglify-es). |
And can you push your package-lock.json changes, as it helps seeing dependency changes + boosts installation speed? |
@nileshgulia1, you probably want to give this a rebase before adding your package-lock. |
yarn is good, but I'm not sure we should maintain both lock files... |
@@ -6,7 +6,6 @@ node_js: | |||
install: | |||
- npm install | |||
- npm run build | |||
- npm run build:respec-w3c-common |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add npm run parcel
here, but npm run parcel
never ends on my local environment. Not sure what's happening.
@@ -71,7 +70,8 @@ | |||
"test:headless": "node ./tests/headless.js", | |||
"test:karma": "npm run karma", | |||
"prepare": "npm run snyk-protect", | |||
"prepublish": "npm run snyk-protect" | |||
"prepublish": "npm run snyk-protect", | |||
"parcel": "parcel ./builds/respec-w3c-common.js -d ./builds/output" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build
is omitted: "parcel build ./builds/respec-w3c-common.js -d ./builds/output"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I Have tried adding build before but is repeatedly throws this error:
parcel build ./builds/respec-w3c-common.js -d ./builds/output
🚨 /home/nilesh/respec/builds/respec-w3c-common.js: sym.definition is not a function
at may_modify (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:8931:31)
at TreeTransformer.abort [as before] (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:8541:74)
at AST_VarDef.eval [as transform] (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:4570:35)
at eval (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:4586:25)
at doit (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:130:23)
at MAP (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:156:52)
at do_list (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:4585:16)
at eval (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:4664:28)
at AST_Const.eval [as transform] (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:4573:17)
at collapse (eval at <anonymous> (/home/nilesh/respec/node_modules/uglify-es/tools/node.js:21:1), <anonymous>:8617:39)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, this is the uglify-es problem we met in #1489 that will be fixed in its next release. See mishoo/UglifyJS#2896
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do not use it until new release ,Using --no-minify
removes this error for me. parcel-bundler/parcel#8. What do you think @saschanaz @marcoscaceres ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We ultimately want to minify this, otherwise the bundle size will be huge... so we are kinda stuck until the next Uglify release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-minify
may still be good for debugging purpose, where source mapping is frequently not perfect.
I'm getting error logs when building:
Too hard to understand as it never says what file is in problem. Also, we should be able to get a non-minified version of file:
|
.travis.yml
Outdated
@@ -6,6 +6,7 @@ node_js: | |||
install: | |||
- npm install | |||
- npm run build | |||
- npm run parcel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding npm run parcel
is causing travis to fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saschanaz Should I remove this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is the required step. I think npm run build
should be completely replaced by npm run parcel
, as that's the whole purpose of using parcel.
Currently we are generating transpiled non-concatenated files into Additionally, we can remove direct babel dependency as all those things should be processed by parcel. (BTW, I'm still worrying about recent uglify-es breakage though... Using parcel will lock the dependency, we cannot consider babel-minify then.) |
I'm still super concerned about trying to work with sourcemaps and a fully bundled app (debugging experience is still really terrible), so I'd prefer to keep basic.html... at least, till I'm confident that source mapping support is sane. |
(I'm willing to try tho - super interested to see what comes out of all this) |
Deeply agreed, but that means we should't use parcel at all, we are just adding a new dependency then. |
Yeah, I'm kinda getting the same feeling - and can definitely live with the current setup. I guess eventually (in around 2 years) we will just be able to use ES Modules and native |
For a reference: this 2015 webpack issue webpack/webpack#1194 is still open 😞 |
Lol, it was filed by an old friend on mine! Hi @Stuk! |
ok, I think we should close this. @nileshgulia1, I want to say thanks for doing this investigation. This has been extremely helpful as it helped us understand the problem better. So, even though we aren't merging, we've learned a lot. |
Haha, hi @marcoscaceres 👋 |
Closes #1129
@saschanaz @marcoscaceres .I have closed old PR for some rebasing and conflicting issues.This patch bundles required
js
files which are dependencies in basic.html including w3c common.js to builds directory.