-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Angular: Remove cached NgModules and introduce a global queue during bootstrapping #24982
Angular: Remove cached NgModules and introduce a global queue during bootstrapping #24982
Conversation
@Marklb Thank you so much for figuring out the root cause of the issue. This was the missing peace. I am so grateful that you dived deep into it! I have thought about your solution, which works great. The only thing might be that renderings might happen in no particular order. I have pushed a different approach using queues. WDYT? Then we don't have to rely on |
@valentinpalkovic It is late for me, so I can't try it right now, but I looked at the code from my phone and I do like not having the |
No pressure! I will wait for your proper feedback. I also changed when the compiled modules got reset in the latest commit because otherwise, your newly written test case of mounting two angular components at once would fail. |
code/frameworks/angular/src/client/angular-beta/utils/BootstrapLock.ts
Outdated
Show resolved
Hide resolved
code/frameworks/angular/src/client/angular-beta/utils/BootstrapLock.ts
Outdated
Show resolved
Hide resolved
I updated a test to be more thorough, adjusted the names and wording from "lock" to "queue", and added back the comment explaining why the queue is necessary. Other than getting your opinion on the questions I added about the |
Hi @Marklb Could you try to resolve the conflicts? Then, I will give CI another try. If it still fails, I will take a look! |
@valentinpalkovic The conflicts should be fixed now. |
Great! I will take a look tomorrow at why CI is failing |
I tried to update the tests for vitest, but I am not familiar with vitest, yet. The I am just assuming that is the problem and maybe I need to add something to stop that or that isn't even the problem, but I will look into it later, when I have more time to dig into it. |
Thanks for taking care of it! I try to support you as soon as I have some free resources. But maybe, first of all, it would make sense to understand why the UI Test in Chromatic fails. Regarding the unit test: Can we run the whole test suite and its tasks sequentially? Maybe Vitest offers some options for doing so exclusively for a particular test suite. |
Closes #24813
What I did
Problem: Caching the NgModule per component, causes the NgModule generated for the first story to get used for the following stories, but they may not have generated the same NgModule.
Explanation:
Assume the following is the CSF file:
In the renderer, the following is basically what the NgModule would be for each story:
When using inline rendering to render both stories in a single document, Storybook attempts to render each story, without waiting on the previous to complete. The problem is that Angular only allows a component to be declared by a single NgModule, so we have to clear the compiled component cache after each story's app is initialized.
The solution being used currently, would cache the NgModule by the
component
defined inMeta
. That avoids the error, but breaks the "Secondary" story, becauseBarComponent
is not declared in the NgModule that was cached when the "Primary" story rendered (Assuming the "Primary" story rendered first.).I don't know if the way inline stories render changed and used to wait for completion of each story or if it just wasn't fully implemented as expected.
AbstractRenderer
already has a method that callsɵresetCompiledComponents
and bothCanvasRenderer
andDocsRenderer
call it after a full render. The problem is two render methods can be running in parallel, which makes the reset not work correctly.To fix
ɵresetCompiledComponents
not getting called correctly, a queue is introduced to theAbstractRenderer
that prevents callingbootstrapApplication
in parallel. This may have an effect on performance, if a particular story is slow to initialize, but I don't know of another way to avoid conflicts between each Angular application's compiling.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
To see the problem with caching the NgModule,
yarn task --task sandbox --start-from auto --template angular-cli/default-ts
stories/core/decorators/componentWrapperDecorator/decorators.stories.ts
, addtags: ['autodocs']
to the default exportMeta
.ParentComponent
threw an error, because the first story did not declareParentComponent
. With this change all should render correctly.To see the reason for adding the lock, instead of just removing the NgModule caching, you can do the same steps, but comment out
await acquireLock(selector);
in the thebootstrapLock
function incode/frameworks/angular/src/client/angular-beta/utils/BootstrapLock.ts
. Commenting that line would disable the lock and cause the multiple modules declaring component error that the caching was supposed to fix.Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
Make sure this PR contains one of the labels below:
Available labels
bug
: Internal changes that fixes incorrect behavior.maintenance
: User-facing maintenance tasks.dependencies
: Upgrading (sometimes downgrading) dependencies.build
: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup
: Minor cleanup style change. Will not show up in release changelog.documentation
: Documentation only changes. Will not show up in release changelog.feature request
: Introducing a new feature.BREAKING CHANGE
: Changes that break compatibility in some way with current major version.other
: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/core
team here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>