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

chore: add sample on how to use esm on cjs #14051

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Oct 1, 2024

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

no examples on how to use ESM-only packages with nestjs (CJS apps), although not directly related with nestjs

What is the new behavior?

this is a first stage to address the issue nestjs/docs.nestjs.com#3093

After creating that new page, I'll mention about these samples as an working examples on three ways to use ESM-only packages in our CJS projects: importing the package when needed or declaring it like a custom provider, or using the --experimental-require-module nodejs flag

About the sample 35: I'm not sure on keeping a dedicate sample because it is just a matter of using a nodejs flag. And we can mention this in the docs. This flags has few gotchas (see https://nodejs.medium.com/announcing-a-new-experimental-modules-1be8d2d6c2ff)

Does this PR introduce a breaking change?

  • Yes
  • No

@coveralls
Copy link

coveralls commented Oct 1, 2024

Pull Request Test Coverage Report for Build 623fb4ab-f46c-4d69-b132-79d31a3d20bd

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.213%

Totals Coverage Status
Change from base Build 9327011d-4e25-476d-80b9-dcb00dcd9e2d: 0.0%
Covered Lines: 6750
Relevant Lines: 7320

💛 - Coveralls

@micalevisk
Copy link
Member Author

micalevisk commented Oct 1, 2024

CI is failing because it's using node16 while the new sample will only work on node18 because of Jest ESM support:

image

not sure what I can do as we do want to test against node16 as well to ensure that nestjs is still compatible with it I guess. I'll see if could ignore that sample when nodejs version is 16

@Apidcloud
Copy link

Would it be possible to make this work for nestjs + swc as well?

@micalevisk micalevisk marked this pull request as draft October 4, 2024 00:15
@micalevisk
Copy link
Member Author

@Apidcloud it's just the same as sample 34 or 35. I just tested it.

@micalevisk micalevisk marked this pull request as ready for review October 27, 2024 20:39
@Apidcloud
Copy link

I remember that my issue a couple of weeks ago with SWC was the dynamic imports (i.e., the node require module experimental flag also worked for me).

But from the example you provided, is this the way to make it work with a dynamic import instead of relying on the experimental flag? They already unflagged it on Node 23 and might backport it to Node 22 on a minor, so it's not a big deal, I guess.

/**
 * This is the same as `import()` expression that is supposed to load ESM packages while
 * preventing TypeScript from transpiling the import statement into `require()`.
 */
export const importEsmPackage = async <ReturnType>(
  packageName: string,
): Promise<ReturnType> =>
  new Function(`return import('${packageName}')`)().then(
    (loadedModule: unknown) => loadedModule['default'] ?? loadedModule,
  );

@micalevisk
Copy link
Member Author

@Apidcloud from what I tested, yes. Having a function like importEsmPackage is the only way due to how the typescript project is configured.

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.

4 participants