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 dependencies #200

Merged
merged 4 commits into from Dec 8, 2022
Merged

Conversation

justingrant
Copy link
Contributor

@justingrant justingrant commented Dec 8, 2022

  • Update to TS 4.9 and fix resulting issues
  • Add casts to prevent TS errors showing up when debugging apps that step into polyfill code
  • switch to @rollup/plugin-terser plugin
  • add version override for rollup-plugin-sourcemaps (verified in source that this seems OK) to avoid peer dependency warnings on build
  • Added JSON type assertion for importing package.json, because otherwise there were build errors.
    Instead of a type assertion (which failed in older Node versions), switched to using a downlevel-compatible way to load package.json. After we no longer need to support Node 14, we can switch back to using imports with type assertions.
  • Switched to using default imports for all rollup plugins
  • Added --bundleConfigAsCjs rollup CLI option. See [v3.0] Always try to load config files via Node if possible rollup/rollup#4621 for context.

* Update to TS 4.9 and fix resulting issues
* Add casts to prevent TS errors showing up when debugging apps that
  step into polyfill code
* switch to @rollup/plugin-terser plugin
* add version override for rollup-plugin-sourcemaps (verified in source
  that this seems OK)
* Added JSON type assertion for importing package.json, because
  otherwise there were build errors.
* Switched to using default imports for all rollup plugins
@12wrigja
Copy link
Contributor

12wrigja commented Dec 8, 2022

Seems that parts of the CI don't like the JSON assert.

Maybe we should split the build and test actions in the CI config so that all the build steps use the same consistent Node env (17?) and then the test steps use whatever relevant version of Node (for most non-test actions, same as the version above, for test actions whatever version we are aiming to test against)?

@12wrigja
Copy link
Contributor

12wrigja commented Dec 8, 2022

Instead of worrying about backwards-compatibility for Rollup, I think we should only ever run it on a recent Node version.

#202 should fix the issues with Node14 tests without having to use any backward-compat stuff.

It's weird that Rollup doesn't like our config because it's not CJS...

@justingrant
Copy link
Contributor Author

It's weird that Rollup doesn't like our config because it's not CJS...

I researched this. The issue (if I understand it correctly which I very well might not be!) is that some newer rollup plugins are ESM-only, and they seem to have trouble loading configs in pre-Node17 environments because in those environments, .js files are assumed to be CJS. I'm sure I'm mangling the explantation, but in any case there's a --bundleConfigAsCjs CLI option when running rollup which seems to fix the problem. More info: rollup/rollup#4621

Maybe we should split the build and test actions in the CI config so that all the build steps use the same consistent Node env (17?) and then the test steps use whatever relevant version of Node (for most non-test actions, same as the version above, for test actions whatever version we are aiming to test against)?
...
Instead of worrying about backwards-compatibility for Rollup, I think we should only ever run it on a recent Node version.
#202 should fix the issues with Node14 tests without having to use any backward-compat stuff.

@12wrigja whoops, I just saw these comments after I checked in workarounds for both issues. I'm open to your proposals for both of these. My only concern is a bit of chicken-and-egg where this PR introduces new dependencies and your PRs change how the build process works but don't validate against these new dependencies. Someone needs to validate that your build process changes work against these new deps.

Given that this PR is currently all green, should we merge it (including the backwards-compat Node14 and CJS config workarounds) and then your build-process PRs can revert those workarounds assuming that they fix everything? Or should we merge those other PRs first and them remove the workarounds from this one? (Assuming they work with these new deps.)

I'm OK either way, although I have a slight preference for merging this PR as-is because I'm lazy. :-)

@12wrigja
Copy link
Contributor

12wrigja commented Dec 8, 2022

Feel free to merge this as-is and I'll play around with just how much is fixed by my other PR.

@justingrant justingrant merged commit c65455a into js-temporal:main Dec 8, 2022
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.

None yet

2 participants