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] Preview types PR causes ember-auto-import CI to fail #20190

Closed
mansona opened this issue Sep 9, 2022 · 8 comments
Closed

[Bug] Preview types PR causes ember-auto-import CI to fail #20190

mansona opened this issue Sep 9, 2022 · 8 comments
Labels
Bug TypeScript Work on Ember’s types

Comments

@mansona
Copy link
Member

mansona commented Sep 9, 2022

🐞 Describe the Bug

I'm deep on a rabbit hole to fix something in ember-auto-import and I've hit another unrelated CI breakage 😞 Essentially some of ember-auto-import's (large) CI matrix is broken and I've tracked it down to the first commit in this PR: #20180 using git bisect:

➜  ember.js git:(6278a8b7c) git bisect bad
6278a8b7cfe55ef2b138ec7d1324bc5cc79544c3 is the first bad commit
commit 6278a8b7cfe55ef2b138ec7d1324bc5cc79544c3
Author: Chris Krycho <redacted@redacted.redacted>
Date:   Mon Aug 22 21:32:04 2022 -0600

I don't understand the error or why adding types might cause this particular problem in this particular test but this is what it is complaining about:

message: |
    Build Error (broccoli-persistent-filter:Babel > [Babel: ember-source]) in @ember/-internals/bootstrap/index.js
    
    [BABEL]: Cannot find module '@babel/core'
    Require stack:
    - /tmp/tmp-[18](https://github.com/ef4/ember-auto-import/runs/8259214273?check_suite_focus=true#step:6:19)35QNd8rBD1GlmJ/node_modules/ember-source/node_modules/@babel/plugin-transform-block-scoping/lib/tdz.js
    - /tmp/tmp-1835QNd8rBD1GlmJ/node_modules/ember-source/node_modules/@babel/plugin-transform-block-scoping/lib/index.js
    - /home/runner/work/ember-auto-import/ember-auto-import/node_modules/@babel/core/lib/config/files/module-types.js
    - /home/runner/work/ember-auto-import/ember-auto-import/node_modules/@babel/core/lib/config/files/configuration.js

You can see the full CI error here

🔬 Minimal Reproduction

You can use the ember-auto-import CI suite to test this.

I was able to run git bisect on ember.js for this by replacing the dependency line for ember-source-canary in test-scenarios/package.json for "ember-source-canary": "file:/Users/mansona/git/opensource/ember/ember.js", (i.e. my local check of ember.js). For some reason you still need to run npm i in the root folder of ember-auto-import if you're going to do this

😕 Actual Behavior

I think it's a runtime error since it's being reported as a qunit error 🤔 which leads me to believe that something that should be transpiled isn't getting transpiled

🤔 Expected Behavior

No errors, build or runtime 🤷

🌍 Environment

  • Ember: Canary (at time of writing)
  • Node.js/npm: Node: v14.16.0 npm: 8.3.0
  • OS: macOS and ubuntu (on CI)
  • Browser: N/A (just running ember test on command line)

➕ Additional Context

@lolmaus
Copy link
Contributor

lolmaus commented Sep 9, 2022

CC @chriskrycho

@chriskrycho
Copy link
Contributor

Best guess would be the @glimmer/component entry in devDependencies and peerDependencies. Otherwise, it’s possible it’s trying to run Babel on the files in types. I’m on vacation till Monday but will help then if this is unresolved.

@chriskrycho chriskrycho added Bug TypeScript Work on Ember’s types labels Sep 12, 2022
@chriskrycho
Copy link
Contributor

I just tested the hypothesis above by removing the peerDependencies entry from ember-source's package.json, and it does indeed pass once I remove that. I’ll circle up with @ef4 and see what our path forward needs to be.

@ef4
Copy link
Contributor

ef4 commented Sep 13, 2022

I think this is only a test infrastructure problem in ember-auto-import. It looks likely that

  • if package X has a peer dep
  • and package X depends on package Y
  • and package Y has a peer dep satisfied from a place higher than package X in the node_modules tree

then scenario tester has a bug. This scenario hadn't come up until ember-source added its first peer dep.

@ef4
Copy link
Contributor

ef4 commented Sep 13, 2022

Incidentally, I see that ember-source depends on @babel/plugin-transform-block-scoping but fails to satisfy that package's peerDep on @babel/core.

Most of the time that works by accident, because another package (ember-cli-babel) has its own dependency on @babel/core and npm decides to hoist @babel/core into a place where @babel/plugin-transform-block-scoping happens to be able to see it. But it's pretty fragile! There is no path of declared dependencies that actually communicates to users or the package manager that to use ember-source you must have @babel/core.

(I don't actually think somebody should rush into fixing that. Because I think ember-source should instead drop the @babel/plugin-transform-block-scoping dependency entirely. Because it shouldn't be providing ES modules via treeForVendor where such manual shenanigans are required. Modules in treeForAddon would get that plugin automatically.)

@mansona
Copy link
Member Author

mansona commented Sep 13, 2022

Ok cool it sounds like you have a lead to what the exact issue is then @ef4 which is great! But I'm not exactly clear what the next steps are here 🤔

I guess the first step is that we should probably close this issue and open a new one in the relevant repo that has more context on what the actual issue is and what some possible steps for mitigation might be?

@ef4
Copy link
Contributor

ef4 commented Sep 13, 2022

Yes, we can close this issue.

The bug is probably in https://github.com/stefanpenner/node-fixturify-project, specifically in the package linking code that I added there.

@ef4
Copy link
Contributor

ef4 commented Oct 13, 2022

After investigating further we think there's no bug in node-fixturify-project and this was really caused by the "incidentally" stuff I mentioned above. There is work ongoing to solve that more permanently with the release of ember-cli-babel 8.

emberjs/ember-cli-babel#453

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug TypeScript Work on Ember’s types
Projects
None yet
Development

No branches or pull requests

4 participants