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

feat: allow serverless configuration with .cjs and .mjs extensions #11586

Merged
merged 3 commits into from Dec 12, 2022

Conversation

RianFuro
Copy link
Contributor

@RianFuro RianFuro commented Dec 7, 2022

This commit adds support for .cjs and .mjs file extensions on the serverless configuration file. If defined as serverless.mjs, the file will be sourced using dynamic import instead of require.js.

This encompasses a minimal viable change to allow the user to use es-modules in his codebase. While we don't check the user's package.json for the package type yet, it should still allow using an es-module project by simply setting the configuration extension to .cjs or .mjs explicitly.

Addresses: #11039
Relates To: #11147

Notes

I'm following up on the conversation in the related PR and only provide minimal changes that allow serverless to operate in a type: module project without major issues. By recognizing .cjs and .mjs extensions we at least give the user full control on how he wants his configuration to be handled.

I left out auto-detection via the user's package.json mostly because I can't be bothered, the proposed changes are more than enough for me.

I read the notes on testing, however I believe it doesn't really apply to these changes? Hence the supplied tests are mostly for completion's sake and for satisfying the coverage checks.

I found a few more references to file-type detection in lib/cli/triage.js, but I can't really make sense of what's happening there. If any changes are needed here, a few pointers would be appreciated.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@RianFuro that looks great! I have just one comment, also it'll be nice for completeness (but in other PR) add support to ESM for .js case.

lib/configuration/read.js Outdated Show resolved Hide resolved
This commit adds support for .cjs and .mjs file extensions on the serverless
configuration file. If defined as `serverless.mjs`, the file will be sourced
using dynamic import instead of require.js.

This encompasses a minimal viable change to allow the user to use es-modules
in his codebase. While we don't check the user's package.json for the package
type yet, it should still allow using an es-module project by simply setting
the configuration extension to .cjs or .mjs explicitly.
@RianFuro
Copy link
Contributor Author

Hey @medikoo, just checking, but is there anything else you'd need me to do to accept the PR?

@medikoo
Copy link
Contributor

medikoo commented Dec 12, 2022

@RianFuro once the update is ready, be sure to re-request review (in top right box)

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @RianFuro !

lib/configuration/read.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Base: 85.63% // Head: 85.62% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (32f6569) compared to base (e791e04).
Patch coverage: 71.42% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11586      +/-   ##
==========================================
- Coverage   85.63%   85.62%   -0.01%     
==========================================
  Files         315      315              
  Lines       13134    13140       +6     
==========================================
+ Hits        11247    11251       +4     
- Misses       1887     1889       +2     
Impacted Files Coverage Δ
lib/configuration/read.js 82.27% <66.66%> (-1.29%) ⬇️
lib/cli/resolve-configuration-path.js 96.87% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Looks great @RianFuro !

@medikoo medikoo merged commit 9d57933 into serverless:main Dec 12, 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