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

Error using ember-moment since 2.6.2 #578

Closed
dwickern opened this issue May 1, 2023 · 12 comments
Closed

Error using ember-moment since 2.6.2 #578

dwickern opened this issue May 1, 2023 · 12 comments

Comments

@dwickern
Copy link

dwickern commented May 1, 2023

Ember-moment uses macro conditions to decide which moment implementation to import:

https://github.com/adopted-ember-addons/ember-moment/blob/71cc4a1a3e12a912a56cc657c97170e3a7be0483/src/index.js

if (macroCondition(dependencySatisfies('moment-timezone', '*'))) {
  return importSync('moment-timezone').default;
} else if (macroCondition(dependencySatisfies('moment', '*'))) {
  return importSync('moment').default;
}

The first condition evaluates true, even though there is no dependency on moment-timezone. There is only the optional peer dependency in ember-moment's own package.json.

Reproduction: https://github.com/dwickern/ember-moment-396-reproduction
Related ember-moment issue: adopted-ember-addons/ember-moment#396

@NullVoxPopuli
Copy link
Contributor

NullVoxPopuli commented May 2, 2023

Does the issue still occur if you use pnpm in your reproduction?

Both older npm and yarn are ceally bad at dealing with peers.

Also, what's your npm version?

@mkszepp
Copy link

mkszepp commented May 2, 2023

@NullVoxPopuli i was running today in the same error like @dwickern .

We use npm v9.5.0 with node v18.14.2.

We have installed moment

npm ls moment

[email protected] /home/app
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected]

@NullVoxPopuli
Copy link
Contributor

Does auto import 2.6.3 fix anything for you? It reverts the change to defaults in 2.6.2

@mkszepp
Copy link

mkszepp commented May 2, 2023

i don't think so... because i have manually set ^2.6.3 in my package.json, removed package.lock & node_modules folder

Update: tested again, the latest working version is atm 2.6.1

@dwickern
Copy link
Author

dwickern commented May 2, 2023

My app uses yarn:

% yarn --version
3.2.2
% yarn why moment
└─ ui@workspace:.
   └─ moment@npm:2.29.4 (via npm:^2.29.4)
% yarn why moment-timezone

The reproduction was using npm:

% npm --version
8.18.0
% npm ls moment
[email protected] /Users/dwickern/code/test/ember-moment-396-repro
├─┬ [email protected]
│ └── [email protected] deduped
└── [email protected]
% npm ls moment-timezone
[email protected] /Users/dwickern/code/test/ember-moment-396-repro
└── (empty)

I converted the reproduction to pnpm and it has the same issue:

% pnpm --version
8.3.1
% pnpm why moment
Legend: production dependency, optional only, dev only

[email protected] /Users/dwickern/code/test/ember-moment-396-repro

devDependencies:
ember-moment 10.0.0
└── moment 2.29.4 peer
moment 2.29.4
% pnpm why moment-timezone

The reproduction is using ember-auto-import 2.6.3. It works if I downgrade to 2.6.1.

@ef4
Copy link
Collaborator

ef4 commented May 2, 2023

Thanks for the reproduction.

The macroCondition is actually not the problem here, when I look at the build output of the reproduction I see that it would go down the moment branch and not the moment-timezone branch.

The problem is that we aren't even getting that far. The bug fix in #574 treats dependencies that webpack can't find as things that should be looked up at runtime in the classic AMD loader, and declares them as static dependencies so that the module loader can evaluate them in the right order. But it's doing that accidentally for the optional dependency here, making it not-optional.

You will also notice that this bug only strikes in development. If you run the reproduction with ember s --environment=production it works. That's because in prod we fully optimize away the dead branch, even before webpack has a chance to see it. We don't do that in development because in general some macros want to have runtime implementations in development (although dependencySatisfies is not really one of them, and if we had a more sophisticated implementation of macroCondition it could do this particular branch elimination even in dev).

Until we have a fix for this, here is a workaround that also fixes the reproduction app:

diff --git a/ember-cli-build.js b/ember-cli-build.js
index ed991bd..d5aa0c0 100644
--- a/ember-cli-build.js
+++ b/ember-cli-build.js
@@ -1,9 +1,18 @@
 'use strict';
 
 const EmberApp = require('ember-cli/lib/broccoli/ember-app');
+const webpack = require('webpack');
 
 module.exports = function (defaults) {
   const app = new EmberApp(defaults, {
+    autoImport: {
+      webpack: {
+        plugins: new webpack.IgnorePlugin({
+          // workaround for https://github.com/embroider-build/ember-auto-import/issues/578
+          resourceRegExp: /moment-timezone/,
+        }),
+      },
+    },
     // Add options here
   });

@mkszepp
Copy link

mkszepp commented May 3, 2023

@ef4 thanks, confirm that the workaround has solved the problem

@mehran-naghizadeh
Copy link

mehran-naghizadeh commented May 23, 2023

This workaround breaks the ember serve command, in my case with ember-source 3.28. The issue is with module being null in this loop:

function gatherExternals(module, output = new Set()) {
  if (externals.has(module)) {
    ...
  }
  else {
    ...
    for (let dep of module.dependencies) {
      ...
    }
    ...
  }
  ...
}

.../node_modules/ember-auto-import/js/webpack.js

@kategengler
Copy link

The workaround solved this today in an app using ember-source 4.4

@mehran-naghizadeh
Copy link

Still struggling with this issue, I want to share my case. I have replaced moment.js with day.js, hoping that I will just escape the issue. Now I get the following error message, though:

Uncaught Error: Could not find module dayjs imported from ...

When I remove that import line (along with other stuff associated with it), I get yet another similar error:

Could not find module @formatjs/intl imported from ...

Apparently then, the issue is not specific to moment.

@NullVoxPopuli
Copy link
Contributor

@mehran-naghizadeh are these dependencies that you're importing declared in your package.json? A reproduction would be most helpful ✨

@ef4
Copy link
Collaborator

ef4 commented Aug 16, 2023

I think that sounds like an unrelated issue.

The underlying issue here about those optional peer deps becoming non-optional should have been fixed in @embroider/macros 1.11.1 due to embroider-build/embroider#1468.

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

No branches or pull requests

6 participants