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

Allow generating ESM output from npm #1494

Closed
wants to merge 1 commit into from

Conversation

aldenquimby
Copy link
Contributor

@aldenquimby aldenquimby commented Jun 7, 2023

What did you implement:

Closes #1493

How did you implement it:

How can we verify it:

  • my project works when I include this change locally
  • generating .js file allows serverless invoke local to work, and including type: module allows the deployed Lambda to work:
image

Todos:

  • Write tests - packExternalModules.test.js already ensures copyPackageSectionNames works
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: YES

@@ -31,7 +31,7 @@ describe('npm', () => {
});

it('should return no packager sections', () => {
expect(npmModule.copyPackageSectionNames).toEqual([]);
expect(npmModule.copyPackageSectionNames).toEqual(['type']);
Copy link
Member

@vicary vicary Jun 13, 2023

Choose a reason for hiding this comment

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

The test name does not reflect your change,

  1. If you want to make it a minor update, the test should stay to give confidence of backward compatibility.
  2. If it's impossible, remove this test case and make it a major update.

Also if we are talking about ESM, shall we also copy other related sections such as "main", "exports"? I don't have an exhaustive list of sections to be copied, but you get the idea.

Copy link
Contributor Author

@aldenquimby aldenquimby Jun 13, 2023

Choose a reason for hiding this comment

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

Ok great. Makes sense re:all ESM sections.

Do you have any ideas for how to make this non-breaking without adding a configuration option? Non-breaking was my main goal with the alternate PR

Copy link
Member

Choose a reason for hiding this comment

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

AFAICT copying more sections from package.json should not affect backward compatibility, such that cjs only versions of node.js won't understand esm sections.

But you may want to take a deeper look for potential cases where newer versions of Node.js may incorrectly interpret ESM modules into CJS, this could happen for older modules. My gut feeling is not likely, I'd like your double confirm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the list of fields is: type, exports, main, and types (the last two are CJS fallbacks for exports). Sources: node and ts. imports could arguably be included too, but it's internal to a package and if you're using webpack then you shouldn't need imports in a build artifact

None of these are safely backwards compatible though. If I copy type in my existing project today, without also reworking my webpack config and a few other build tools, I hit this error in Lambda:

ReferenceError: require is not defined in ES module scope, you can use import instead

This file is being treated as an ES module because it has a '.js' file extension and '/var/task/package.json' contains \"type\": \"module\". To treat it as a CommonJS script, rename it to use the '.cjs' file extension.

You are correct that older versions of Node would work fine, but for anyone on Node 14+, this would be breaking. And I imagine that's most users because Node 12 went EOL in 2020 with security support ending in 2022.

So if we want this to be backwards compatible, I think we should go with the other PR, and I can update the example to copy all 4 of the ESM-related fields

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense, we can close this and move to the other one.

@aldenquimby aldenquimby deleted the fix-1493 branch June 20, 2023 14:20
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.

Allow generating ES Module (ESM) output from npm
2 participants