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

Angular: Add random attribute to bootstrapped selector #24972

Merged

Conversation

Marklb
Copy link
Member

@Marklb Marklb commented Nov 24, 2023

Closes #23875

What I did

Each story needs a unique selector to bootstrap the Angular app. #15501 had appended a unique id to the storyId, but when the prepareForInline function was removed, that part of it must not have been moved to a new place. I went with a similar idea, but I did it a little different. Using the same nanoid dependency, I added an attribute that is set to the storyId with a unique string appended. This attribute is then added to the element that will be bootstrapped as an attribute. The boostrap selector changes from storyId to storyId[uniqueAttribute].

I did this in the AbstractRenderer, so it applies to all stories, but I think it is only needed by the DocsRenderer. If we don't want the CanvasRenderer to use the attribute in it's selector then I can move that attribute initialization to the DocsRenderer.

I tried to preserve the storyId, in case something is using it, but I didn't think about it causing two id DOM attributes to be the same. If that should not happen then I can try to find a spot to adjust the id, like the prepareForInline function used to do, but I didn't see a clear equivalent.

I also added a test for this, since there didn't seem to be one.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template angular-cli/default-ts
  2. Add an MDX file that renders two of the same story, e.g. <Story of={ButtonStories.Primary}/><Story of={ButtonStories.Primary}/>
  3. Open Storybook in your browser
  4. Access the MDX doc and the story should be rendered twice

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/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>

@yannbf
Copy link
Member

yannbf commented Nov 30, 2023

Quick question: Will this change impact snapshot testing? Given that the id is always going to be inconsistent

@yannbf yannbf changed the title [Angular] Add random attribute to bootstrapped selector Angular: Add random attribute to bootstrapped selector Nov 30, 2023
@valentinpalkovic
Copy link
Contributor

I will check this.

@Marklb
Copy link
Member Author

Marklb commented Nov 30, 2023

I am not too familiar with how the snapshot testing works, but possibly. The reason I went with nanoid is because the previous implementation, that this is restoring, used it. This implementation applies it to the AbstractRenderer, instead of just in docs. So, it may have more affect on that than the previous would have had.

One reason I didn't try to only apply it to the docs, is to avoid this error from any way the renderer is used.

Maybe an incremental counter could be used, instead of a random value. It may be predictable enough and avoid a dependency.

@valentinpalkovic
Copy link
Contributor

I think that applying the logic to the DocsRenderer should be sufficient to fix the bug and circumvent any possible snapshot-related issues in the normal Story view. Let me know, whether you want to change it. I can also take care of it if you want :)

@Marklb
Copy link
Member Author

Marklb commented Dec 1, 2023

@valentinpalkovic I can probably change it tomorrow, unless you want to go ahead and take care of it.

@Marklb
Copy link
Member Author

Marklb commented Dec 2, 2023

I changed the implementation. It now only affects DocsRenderer and just appends an incrementing counter, per story id. I also included a reset function to clear the counters for scenarios, like testing, that may want to reset the counters. The nanoid dependency isn't needed this way, either.

@valentinpalkovic valentinpalkovic merged commit 8a9dce4 into storybookjs:next Dec 12, 2023
47 of 48 checks passed
@github-actions github-actions bot mentioned this pull request Dec 12, 2023
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot Render Same Story Twice in Docs (Angular)
3 participants