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

CJS / ES Module? #137

Closed
Tracked by #636
jycouet opened this issue Mar 10, 2022 · 12 comments · Fixed by #144
Closed
Tracked by #636

CJS / ES Module? #137

jycouet opened this issue Mar 10, 2022 · 12 comments · Fixed by #144

Comments

@jycouet
Copy link

jycouet commented Mar 10, 2022

I'm using reflect-metadata in a SvelteKit project, and it's working great 👍

I have a warning at the start:
reflect-metadata doesn't appear to be written in CJS, but also doesn't appear to be a valid ES module (i.e. it doesn't have "type": "module" or an .mjs extension for the entry point). Please contact the package author to fix.

Maybe this message speaks better to you?
Is it the way you distribute the package?

@geongeorge
Copy link

Same error for me when trying to add nest js to a vite project

@dstampher
Copy link

I am new to TS. I am using typeorm, which requires this library. I cannot set tsconfig module type to esnext, it must be commonjs when I import "reflect-metadata". And that is causing other problems in my application that are very annoying for a newbie.

@patrickjm
Copy link

I got this to work importing from a package with "type": "module" and using yarn v3 / berry:

Create this patch file:

diff --git a/package.json b/package.json
index 11d979c..e07509d 100644
--- a/package.json
+++ b/package.json
@@ -4,6 +4,7 @@
   "description": "Polyfill for Metadata Reflection API",
   "main": "Reflect.js",
   "types": "index.d.ts",
+  "type": "module",
   "typescript": {
     "definition": "Reflect.d.ts"
   },

Then in your package json, add this resolutions block:

"resolutions": {
    "reflect-metadata": "patch:[email protected]#./path/to/your/patchfile.patch"
  },

If you are using a monorepo, this has to be at the top level package.json regardless of where you are importing reflect-metadata.

@ljharb
Copy link

ljharb commented Sep 26, 2022

Nothing should ever need type:module; if it's ESM it'd have .mjs, so I'm not sure why vite is failing to properly recognize https://unpkg.com/browse/[email protected]/Reflect.js as CJS.

It doesn't export anything, since it just mutates the global Reflect, but since that's not a requirement of CJS, this seems like a vite bug, plain and simple.

@jycouet
Copy link
Author

jycouet commented Nov 5, 2022

You know probably way better than me the cjs / mjs issue in general.

If I understand it correctly:

  • if your file is .cjs everyone knows that it's a commonjs file.
  • if your file is .mjs everyone knows that it's a esm file.

Now, if you have a .js no one knows... it depends on your package.json to select what is your "default" way of doing.

More and more lib ship in both mode: esm & cjs & a default with these kind of exports:
image

Like this, tooling can pickup the right things for them.

@AirMonkeys-Kun
Copy link

reflect-metadata doesn't appear to be written in CJS, but also doesn't appear to be a valid ES module (i.e. it doesn't have "type": "module" or an .mjs extension for the entry point). Please contact the package author to fix.

@rbuckton
Copy link
Owner

rbuckton commented Nov 27, 2022

reflect-metadata doesn't appear to be written in CJS, but also doesn't appear to be a valid ES module (i.e. it doesn't have "type": "module" or an .mjs extension for the entry point). Please contact the package author to fix.

reflect-metadata is actually written as a Script, not a Module. It does not use import or export (which would imply ESM/MJS), nor does it use module, exports, or require() (which would imply CJS). Instead, it is written so as to augment the global Reflect object.

Since it is written as a Script, and has no syntactic or runtime dependencies on a specific module syntax, it is both a perfectly legal CJS module and a perfectly legal ES module (though NodeJS will only treat it as CJS). It can be loaded in any of the following ways:

  • In NodeJS/CommonJS via require("reflect-metadata")
  • In NodeJS/ESM via import "reflect-metadata" (because a NodeJS module can import a CJS file)
  • In the Browser via <script src="path/to/reflect_metadata/Reflect.js"></script> (because it's a Script)
  • In the Browser/ESM via import "path/to/reflect-metadata/Reflect.js"

This package obeys the mechanics of NodeJS's module resolution algorithm. If you are using a bundler that is unable to bundle this package, then it is not obeying the mechanics of NodeJS's module resolution algorithm, and thus the bug lies in the bundler. I would advise you open an issue in your bundler's repo.

@tobiasdiez
Copy link

@rbuckton I agree with your view, but could you please add an exports config in package.json. This would help bundlers to understand the nature of the package and is recommended practice anyway: https://nodejs.org/api/packages.html#conditional-exports

@ljharb
Copy link

ljharb commented Mar 4, 2023

Bundlers should be perfectly capable of figuring out the nature of the package without the exports field; browserify's been doing it for a decade. If newer tools can't keep up with standards that old and time-tested, you may want to avoid them.

@alessiostalla
Copy link

@ljharb you can't avoid them if the framework you're using mandates them. For example, Angular warns that

⠸ Building...▲ [WARNING] Module 'reflect-metadata' used by '../node_modules/@strumenta/tylasu/dist/esm/model/model.js' is not ESM

While the above isn't blocking, it will still cause concerned devs to spend time looking for a cause and, hopefully, end up here. When that would be easily avoided by a couple of declarations in package.json, I don't see why they shouldn't be added.

@ljharb
Copy link

ljharb commented Dec 7, 2023

Sounds like a flaw/mistake/bug in angular then.

@jycouet
Copy link
Author

jycouet commented Dec 7, 2023

Each time I see an update on this ticket I have mixed feelings (I know, no one cares 😅)

The root cause is this small thing called vite/rollup that all most frameworks are using. (last one coming was Angular 🎉)

This one change in this library is not solving the root cause yes, but solving all warnings.
Not everyone is as good as you @ljharb!

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 a pull request may close this issue.

9 participants