Skip to content
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

Update build dependencies #690

Merged
merged 5 commits into from
Jun 22, 2020
Merged

Update build dependencies #690

merged 5 commits into from
Jun 22, 2020

Conversation

justingrant
Copy link
Collaborator

@justingrant justingrant commented Jun 20, 2020

While troubleshooting #687, I ended up updating all build dependencies except demitasse. Updating didn't fix the problem but all builds and tests still worked fine, so while I was in the neighborhood I figured I'd turn that work into a PR instead of rolling back. Here's what's in it:

  • prettier->v2 - required a few minor spacing/line-break changes, but otherwise no changes
  • rollup->v2 - Sped up builds by a few seconds (yay!). Also replaced deprecated rollup plugins.
  • replaced uglify with terser, which had a supported rollup 2.x plugin (and can minify ES6 code if we want to do that later) while uglify plugin didn't seem to be rollup 2.x-friendly
  • added --cache option to lint step, which makes linting ~5x faster after the first run
  • moved all dev dependencies into dev section in package.json (there were a few dev stragglers in the main deps section)
  • Excluded more generated code from lint and .gitignore

Notable updates:
- prettier->v2
- rollup->v2 (sped up builds by a few seconds)
- replaced deprecated rollup plugins
- replaced uglify with terser for rollup 2.x
- added --cache option to lint step (5x faster)
- moved dev dependencies into dev section in package.json
Also: allow @ts* directives which let me verify test code with TSC
- Explicitly added `babelHelpers` option to avoid warning message
- Removed invalid `lib` option
- Switched unglify->terser which has a supported rollup V2 plugin
@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #690 into main will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #690      +/-   ##
==========================================
- Coverage   94.41%   94.41%   -0.01%     
==========================================
  Files          17       17              
  Lines        4780     4778       -2     
  Branches      757      757              
==========================================
- Hits         4513     4511       -2     
  Misses        264      264              
  Partials        3        3              
Flag Coverage Δ
#test262 52.53% <66.66%> (ø)
#tests 89.21% <100.00%> (-0.01%) ⬇️
Impacted Files Coverage Δ
polyfill/lib/ecmascript.mjs 95.63% <100.00%> (-0.01%) ⬇️
polyfill/lib/intl.mjs 98.73% <100.00%> (ø)
polyfill/lib/intrinsicclass.mjs 93.18% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4481a79...abbffaa. Read the comment docs.

@ptomato ptomato requested review from ptomato and ryzokuken June 20, 2020 03:48
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -8,27 +8,27 @@
},
"main": "polyfill/lib/index.mjs",
"dependencies": {
"ecmarkup": "^3.22.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The split between dependencies and devDependencies is a bit weird for the root package anyway. The package that's published to npm is only the polyfill/ directory, and for the rest (spec, documentation, meeting minutes) there really are no runtime dependencies as such.

return Temporal.DateTime.from(dateTime)
.inTimeZone(timeZone, { disambiguation: 'reject' })
.inTimeZone(localTimeZone);
return Temporal.DateTime.from(dateTime).inTimeZone(timeZone, { disambiguation: 'reject' }).inTimeZone(localTimeZone);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad prettier changed their minds on this!

import builtins from "rollup-plugin-node-builtins";
import resolve from "@rollup/plugin-node-resolve";
import {uglify} from 'rollup-plugin-uglify';
import { terser } from 'rollup-plugin-terser';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into switching to terser some time ago and found it didn't work. But now I can't remember why. It seems to work now, so maybe the problem was with rollup v1.x. Let's go ahead with this, we can always revert if it causes problems.

@ptomato ptomato merged commit 8a000a2 into tc39:main Jun 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants