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

fix: compile to commonjs #369

Merged
merged 2 commits into from Mar 31, 2021
Merged

fix: compile to commonjs #369

merged 2 commits into from Mar 31, 2021

Conversation

akash1810
Copy link
Member

What does this change?

TL;DR Publishing @guardian/cdk in ESM format is tricky, it is easiest to stay on CommonJS.

In #365, we changed the output format to ES Modules. This was after #356 which bumped read-pkg-up to v8.0.0.

Looking at read-pkg-up's release notes for v8.0.0 the package has changed to ESM only and consuming projects need to:

  1. Use ESM yourself. (preferred)
    Use import foo from 'foo' instead of const foo = require('foo') to import the package.
  2. If the package is used in an async context, you could use await import(…) instead of require(…).
  3. Stay on the existing version of the package until you can move to ESM.

More information can be found here.

#365 incorrectly did option 1 as "type": "module" was not added to package.json. However, even with that change, it is pretty difficult for consumers of @guardian/cdk to use an ESM release as required tooling such as ts-node (see also this), ESLint and Jest have varying support.

This PR does option 3. That is, publishing @guardian/cdk in ESM format is tricky, it is easiest to stay on CommonJS. We'll have to stay on v7.0.1 of read-pkg-up to achieve this; v8.0.0 doesn't bring any new features or fixes, so this is safe.

Furthermore, after a couple of hours, I couldn't get option 2 to work, even though top level await is supported. We can experiment later.

It's worth noting that #367 plans to introduce an integration test to this repository, which should make experimenting with various module formats with more confidence. Currently, this test will define an empty GuStack and attempt to synth it.

Lastly, although #365 suggests the change was tested locally, I suspect it was fluke that it worked.

Does this change require changes to existing projects or CDK CLI?

No.

How to test

n/a - we're getting back into the same state as we were before #356 and @guardian/cdk v6.0.1.

How can we measure success?

We publish a usable package again!

Have we considered potential risks?

n/a

@akash1810 akash1810 requested a review from a team as a code owner March 31, 2021 07:26
Copy link
Contributor

@jacobwinch jacobwinch left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to explain this in detail 👍

@akash1810
Copy link
Member Author

Thanks for taking the time to explain this in detail 👍

Module formats in Node are complicated! I hope my explanation is correct 😬 !

@akash1810 akash1810 merged commit f8aebdf into main Mar 31, 2021
@akash1810 akash1810 deleted the aa-commonjs branch March 31, 2021 07:52
@github-actions
Copy link
Contributor

🎉 This PR is included in version 6.2.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants