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
Upgrading dependencies and switching build to all rollup #2149
Conversation
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.
If I recall correctly the main reason I didn't do this originally for the extras was because Rollup adds an extra wrapper.
It doesn't seem a very high cost though at all, so I'd be happy to land this.
We must make sure to test IE11 though before landing.
"min": "npm run min:systemjs && npm run min:sjs", | ||
"min:systemjs": "cd dist && terser system.js -c passes=2 -m --source-map --comments \"/SystemJS \\d/\" -o system.min.js", | ||
"min:sjs": "cd dist && terser s.js -c passes=2 -m --source-map -o s.min.js", | ||
"build": "rimraf dist && rollup -c", |
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 retain a build mode / option that allows skipping minification here for quick builds in development.
I seem to recall there may even be a way with RollupJS to pass options, but can't remember if that is correct. Otherwise environment variables can work too.
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.
You can with rollup's --environment
flag
src/system-core.js
Outdated
if (name.__esModule) { | ||
ns.__esModule = name.__esModule; | ||
if (name[__esModule]) { | ||
ns[__esModule] = name[__esModule]; |
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.
This isn't necessary because it shouldn't affect the gzip size.
* Minimal SystemJS Build | ||
*/` : `/* | ||
* SystemJS ${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.
Indentation here needs to shift back two spaces.
I verified that the browser tests work in chrome, ff, safari 13, edge, and IE11, with both system.js and system.min.js. |
Changes:
//# sourceMappingURL
comment to minified output. This accounts for some of the byte size increases.The motivation for this change was a susbsequent change that adds error codes to reduce the bundle size further.
I haven't tested all the minified files in a browser yet, but will do so in ie, safari, ff, and chrome if this becomes ready to merge.