-
Notifications
You must be signed in to change notification settings - Fork 137
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
Back onto real canary releases #2073
base: main
Are you sure you want to change the base?
Conversation
This switches our canary scenarios back to actualy latest published alpha. Previously they had been locked into an older fixed snapshot.
@@ -411,6 +402,12 @@ stage2Scenarios | |||
layout: hbs${'`'}<FirstChoice/>${'`'} | |||
}) | |||
`, | |||
'first-choice.hbs': 'first', | |||
'second-choice.hbs': 'second', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to eliminate non-colocated component usage, but it's causing failures because we currently have a real issue:
Extension search works in depscanning and in normal usage, but not in the dep pre-bundling that happens after depscanning. In this test, an addon uses global template resolution to access this component, which is failing because there's no hbs search.
It's unclear whether esbuild / vite will do a good or bad thing after we fix this, in that we definitely don't want pre-bundled dependencies pulling in any parts of the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also saw this and which is why i needed to do this change
Anyway the tests here in stage2 might need more changes. Maybe in a way that make them independent of vite dep optimization. Which is currently not happening. See this file
Edit: in this case it's actually accessing the app component?
The way here is to externalize it during bundling:
https://github.com/embroider-build/embroider/pull/1876/files#diff-a6c8071691a61acc68be864b34d3f338fd92e9bf5217c2d12403ac6029bca885R58
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we will want to externalize during bundling. But not exactly in the way the code you linked is doing it.
That code is using rough heuristics for isInApp
that are unlikely to be long-term true. We have more precise tracking already to know which modules are in the app.
And when we externalize, we should do it via the request.specifier, not the resolution.filename. Because the way the request maps to the resolution can change during development, and the pre-bundled deps would not react to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few problems with that. The request is coming from an optimised dep and then the embroider resolution will not happen. It would also be hard to trace the dep chunk back to the origin.
What can be done is that we create a new specifier which includes both fromFile and specifier. I will try that out.
I paused working on this to go fix some circular imports in ember itself, which were blocking several of the things here I was trying to debug. |
This switches our canary scenarios back to actualy latest published alpha. Previously they had been locked into an older fixed snapshot.