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

[BUG] [2.5.6] import of npm library gives empty object #11900

Open
trusktr opened this issue Feb 9, 2022 · 11 comments · Fixed by #11954
Open

[BUG] [2.5.6] import of npm library gives empty object #11900

trusktr opened this issue Feb 9, 2022 · 11 comments · Fixed by #11954
Labels
in-discussion We are still discussing how to solve or implement it Project:NPM

Comments

@trusktr
Copy link
Contributor

trusktr commented Feb 9, 2022

Blocker: Unable to run a Meteor app because import of a library returns an empty object, as described in all these threads:

I don't know if this ever worked in previous versions of Meteor, but from the threads we can see the issue celebrated quite a few birthdays already.

I think @benjamn might have the most clue as to what this might be caused by; he architected Meteor's module system.

Meteor 2.5.6
Linux Ubuntu 21.10
Node.js v17.3.1
npm 8.3.1

Reproduction:

git clone git@github.com:trusktr/mapapp.git
cd mapapp
git checkout meteor-issue-11900
npm install
meteor --exclude-archs "web.browser.legacy, web.cordova"

Error output:

W20220209-01:57:29.900(-8)? (STDERR) (node:28466) UnhandledPromiseRejectionWarning: TypeError: dayjs is not a function
W20220209-01:57:29.910(-8)? (STDERR)     at createFixtures (server/imports/fixtures.js:15:40)
W20220209-01:57:29.916(-8)? (STDERR)     at server/main.js:10:2
W20220209-01:57:29.920(-8)? (STDERR)     at /home/trusktr/.meteor/packages/promise/.0.12.0.1vb72xd.6v12++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/meteor-promise/fiber_pool.js:43:40
W20220209-01:57:29.921(-8)? (STDERR) (Use `node --trace-warnings ...` to show where the warning was created)
W20220209-01:57:29.922(-8)? (STDERR) (node:28466) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)
W20220209-01:57:29.929(-8)? (STDERR) (node:28466) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
@trusktr
Copy link
Contributor Author

trusktr commented Feb 9, 2022

Added a reproduction.

@trusktr trusktr changed the title [2.5.6] import of npm library gives empty object [BUG] [2.5.6] import of npm library gives empty object Feb 9, 2022
@eric-burel
Copy link
Contributor

eric-burel commented Feb 9, 2022

@trusktr Awesome that you have a simple reproduction!

For the record, this seems to specifically affect ES Modules (Material UI in my case), and the server build.

I recently hit an unexpected issue recently when exploring ES modules, with NPM packages and Next.js: files with ".js" extensions maybe interpreted differently depending if "type": "module" is enabled or not.

  • If "type": "module" is enabled in package.json, ".js" is treated as ES modules. You have to name CommonJS ".cjs" ideally.
  • If not, eg when using conditional exports instead, ".js" is treated as CommonJS. You have to name ES modules ".mjs" ideally.

Maybe it's related? Like Meteor wrongly consider the package as CommonJS during the server build.

For example DayJS is not explicitely using "type":"module": https://github.com/iamkun/dayjs/blob/dev/package.json nor using ".mjs" extension.

For Material UI the package.json has:

"main": "./node/index.js",
  "module": "./index.js",

@denihs
Copy link
Contributor

denihs commented Feb 9, 2022

Hi @trusktr, thank you for the detailed issue!

I saw that you have the package dayjs as a devDependencies.

I tested moving it to dependencies, then running rm -rf node_modules, then meteor npm install, and it was fixed.

Can you try that?

@denihs denihs added the in-discussion We are still discussing how to solve or implement it label Feb 9, 2022
@trusktr
Copy link
Contributor Author

trusktr commented Feb 9, 2022

Maybe it's related? Like Meteor wrongly consider the package as CommonJS during the server build.

Meteor doesn't understand Node.js ESM or package.json exports fields yet (#10906).

I saw that you have the package dayjs as a devDependencies.

I tested moving it to dependencies, then running rm -rf node_modules, then meteor npm install, and it was fixed.

Ah, moving it from devDependencies to dependencies worked. I didn't remove node_modules ore reinstall, I only moved the entry from devDependencies to dependencies then ran meteor and it worked. This indicates Meteor ignoring the dependency in that case, for some reason.

I think this might be considered a bug because all other tools (Webpack, Rollup, Parcel, etc) all successfully build an app regardless of which field dependencies are in, and this is in tuitive: tools (and node module resolution algorithm) care about what is in node_modules, not what is listed in dev vs non-dev dependencies.

Another way to look at this is that having runtime dependencies in dependencies is namely a thing for libraries that run in Node and need to use Node's lookup algorithm at runtime. In this sense, Meteor is treating an app like a library.

If we were to write a plain Node.js app, with devDependencies, then run npm install and finally node server.js (or similar to start the server), it would work. On this note, it will be intuitive for Meteor to also work the same.

The only time devDependencies are ignored in the Node ecosystem is when npm installing a library, and in that case devDependencies are left behind and do not make it to node_modules.

@eric-burel
Copy link
Contributor

@denihs incredible I would never had imagined this to be the issue! Thanks a lot! It's consistent with my own experience with Material UI, as it was installed as a dev dependency in Vulcan. We use a Meteor app as a kind of a monorepo just for dev, so we have a lot of dev dependencies.

@trusktr
Copy link
Contributor Author

trusktr commented Mar 16, 2022

This needs to be marked as a bug.

@denihs
Copy link
Contributor

denihs commented Mar 18, 2022

Hi @trusktr,

This was fixed on this PR.

We have already released a new version with this code.

Could you test it?

Just update your app with:

meteor update --release 2.7-rc.4

@trusktr
Copy link
Contributor Author

trusktr commented Apr 8, 2022

After performing meteor update to 2.7.1, the above reproduction still fails with an error:

 meteor --exclude-archs "web.browser.legacy, web.cordova"
[[[[[ ~/deleteme/mapapp ]]]]]                 

=> Started proxy.                             
=> Started HMR server.                        
=> Started MongoDB.                           
W20220407-20:53:38.794(-7)? (STDERR) /home/trusktr/.meteor/packages/meteor-tool/.2.7.1.tb7vzh.a2uk++os.linux.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.linux.x86_64/dev_bundle/server-lib/node_modules/fibers/future.js:280
W20220407-20:53:38.799(-7)? (STDERR) 						throw(ex);
W20220407-20:53:38.799(-7)? (STDERR) 						^
W20220407-20:53:38.799(-7)? (STDERR) 
W20220407-20:53:38.799(-7)? (STDERR) Error: Cannot find module "/node_modules/dayjs/esm/index.js". Try installing the npm package or make sure it is not a devDependency.
W20220407-20:53:38.799(-7)? (STDERR)     at Module.useNode (packages/modules-runtime.js:647:11)
W20220407-20:53:38.799(-7)? (STDERR)     at module (packages/modules.js:215:8)
W20220407-20:53:38.799(-7)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:336:7)
W20220407-20:53:38.799(-7)? (STDERR)     at Module.require (packages/modules-runtime.js:238:14)
W20220407-20:53:38.799(-7)? (STDERR)     at Module.moduleLink [as link] (/home/trusktr/.meteor/packages/modules/.0.18.0.1i29cl.iter++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/@meteorjs/reify/lib/runtime/index.js:52:22)
W20220407-20:53:38.800(-7)? (STDERR)     at module (imports/day.js:1:1)
W20220407-20:53:38.800(-7)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:336:7)
W20220407-20:53:38.800(-7)? (STDERR)     at Module.require (packages/modules-runtime.js:238:14)
W20220407-20:53:38.800(-7)? (STDERR)     at Module.moduleLink [as link] (/home/trusktr/.meteor/packages/modules/.0.18.0.1i29cl.iter++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/@meteorjs/reify/lib/runtime/index.js:52:22)
W20220407-20:53:38.800(-7)? (STDERR)     at module (server/imports/fixtures.js:1:1)
W20220407-20:53:38.800(-7)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:336:7)
W20220407-20:53:38.800(-7)? (STDERR)     at Module.require (packages/modules-runtime.js:238:14)
W20220407-20:53:38.800(-7)? (STDERR)     at Module.moduleLink [as link] (/home/trusktr/.meteor/packages/modules/.0.18.0.1i29cl.iter++os+web.browser+web.browser.legacy+web.cordova/npm/node_modules/@meteorjs/reify/lib/runtime/index.js:52:22)
W20220407-20:53:38.800(-7)? (STDERR)     at module (server/main.js:1:1)
W20220407-20:53:38.800(-7)? (STDERR)     at fileEvaluate (packages/modules-runtime.js:336:7)
W20220407-20:53:38.800(-7)? (STDERR)     at Module.require (packages/modules-runtime.js:238:14)
=> Exited with code: 1
=> Your application is crashing. Waiting for file change.
^C

In particular, note that the missing file does exist:

❯ ls -l node_modules/dayjs/esm/index.js
-rw-rw-r-- 1 trusktr trusktr 12274 Apr  7 20:52 node_modules/dayjs/esm/index.js

@trusktr
Copy link
Contributor Author

trusktr commented Apr 8, 2022

Here is a new reproduction that includes the meteor update:

git clone git@github.com:trusktr/mapapp.git
cd mapapp
git checkout meteor-issue-11900-2
npm install
meteor --exclude-archs "web.browser.legacy, web.cordova"

@denihs
Copy link
Contributor

denihs commented Apr 8, 2022

Hi, the solution on the PR was to throw a clear error message:

Try installing the npm package or make sure it is not a devDependency..

Did you make sure the package is not a devDependency?

@bobo96run
Copy link

Hi,

I find it unexpected that Meteor build differentiates between prod and dev dependencies (no other build engine I know does that), but at least the error message makes it more explicit indeed!

Looks like it is also the case in above repro example: https://github.com/LUMECraft/mapapp/blob/meteor-issue-11900-2/package.json#L30

{
	"devDependencies": {
		"dayjs": "^1.10.7"
	}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-discussion We are still discussing how to solve or implement it Project:NPM
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants