-
Notifications
You must be signed in to change notification settings - Fork 11.9k
test(@angular/build): add test with 'vitest' import in browser mode #31783
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
Conversation
| projectName, | ||
| projectSourceRoot: this.options.projectSourceRoot, | ||
| optimizeDepsInclude: this.externalMetadata.explicitBrowser, | ||
| optimizeDepsInclude: this.externalMetadata.explicitBrowser.filter( |
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.
Should I add a comment here?
I think that ideally, we should filter out all entrypoints from explicitBrowser but I can't imagine any valid scenario other then the vitest package import.
| * Allow `vitest` import in browser mode. | ||
| * @see https://github.com/angular/angular-cli/issues/31745 | ||
| */ | ||
| export default async function (): Promise<void> { |
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.
Given we really need to run in browser mode to reproduce the bug, I had to install the packages so I went with an e2e.
Any narrower test would be over-specifying and not provide enough confidence.
|
I'm AFK, I guess that I just have to explicitly choose headless. |
749fc15 to
30352fc
Compare
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 was fixed via #31781 as now only implicit external packages are included of which vitest is not.
The root cause is that Vitest is making a variety changes to the optimizeDeps option internally for browser mode. One of which was adding vitest to entries which then caused an issue if vitest was also added to includes. Oddly, it also appears to add it to excludes which one may assume would take priority over an includes element.
An additional test case would still be useful though.
Ah perfect! I saw the PR later and I was about to give it a try to see if it fixes it. |
ab242c5 to
19b1123
Compare
Closes angular#31745 Actually closed by angular#31781
19b1123 to
ef66314
Compare
This fixes the following error:
'The entry point "vitest" cannot be marked as external'
by excluding vitest from the discovered dependencies handed to optimizeDeps.include
Closes #31745
PR Checklist
Please check to confirm your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #31745
What is the new behavior?
Does this PR introduce a breaking change?
Other information