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 (non-breaking) #1495

Merged
merged 1 commit into from
Jul 25, 2023

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
  • 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?: NO

@aldenquimby aldenquimby force-pushed the fix-1493-alternative branch from ee86fae to b9867ad Compare June 7, 2023 15:46
@aldenquimby aldenquimby changed the title Allow generating ESM output from npm Allow generating ESM output from npm (alternate approach) Jun 7, 2023
@aldenquimby
Copy link
Contributor Author

@vicary @j0k3r what are your thoughts on this PR? Anything I can do to help move it forward?

And apologies if tagging you is not the appropriate next step. I'd be happy to start a CONTRIBUTING.md if you let me know the expected process

@vicary
Copy link
Member

vicary commented Jun 13, 2023

I am more inclined towards #1494 because it's a smaller change, we can discuss more in details there.

@aldenquimby aldenquimby changed the title Allow generating ESM output from npm (alternate approach) Allow generating ESM output from npm (non-breaking) Jun 20, 2023
@aldenquimby
Copy link
Contributor Author

@vicary I closed the other PR and expanded on the README here. Please let me know if there's anything else you'd like to see, thank you!

Copy link
Member

@vicary vicary left a comment

Choose a reason for hiding this comment

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

@aldenquimby Just a few touch ups.

Let's also give it some time for @j0k3r to comment, please also note that we may have some stuck tests that requires resolution from the repo owner before our next release.

lib/packagers/npm.js Outdated Show resolved Hide resolved
lib/packagers/yarn.js Outdated Show resolved Hide resolved
tests/packagers/npm.test.js Outdated Show resolved Hide resolved
tests/packExternalModules.test.js Show resolved Hide resolved
vicary
vicary previously approved these changes Jun 23, 2023
@aldenquimby
Copy link
Contributor Author

Thank you for the help on this @vicary !

README.md Outdated Show resolved Hide resolved
@jfroelich
Copy link

We're very interested in this PR being merged because the lack of support for ESM is a blocker for us on several projects. Any chances of that happening soon?

Copy link
Member

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

This looks good to me, can you just squash all commits?

@j0k3r j0k3r mentioned this pull request Jul 24, 2023
@j0k3r j0k3r added this to the 5.12.0 milestone Jul 24, 2023
@j0k3r j0k3r force-pushed the fix-1493-alternative branch from b45e8bd to 0e373da Compare July 25, 2023 09:37
@j0k3r j0k3r enabled auto-merge July 25, 2023 09:38
@j0k3r j0k3r merged commit 182d680 into serverless-heaven:master Jul 25, 2023
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
4 participants