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

Composition: Verify refs in node #12085

Merged
merged 10 commits into from
Aug 20, 2020
Merged

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Aug 17, 2020

Issue: #11892

What I did

  • ADD the type property from node
  • ADD check in node to fetch the iframe.html
  • REMOVE check for iframe.html in lib/api/modules/refs
  • CHANGE lib/api/modules/refs to use the set type of a ref as a replacement for runtime checking iframe.html
  • CHANGE lib/api/modules/refs to not request stories.json with credentials when the server-side fetch succeeded, because that means it's public.

@ndelangen ndelangen requested a review from tmeasday August 17, 2020 16:33
@ndelangen ndelangen self-assigned this Aug 17, 2020
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is done, but some comments

lib/core/src/server/manager/manager-config.js Outdated Show resolved Hide resolved
lib/api/src/modules/refs.ts Show resolved Hide resolved
lib/api/src/modules/refs.ts Outdated Show resolved Hide resolved
@ndelangen ndelangen added the bug label Aug 18, 2020
@ndelangen ndelangen marked this pull request as ready for review August 18, 2020 12:33
@ndelangen
Copy link
Member Author

Ready for review @rannyeli @TechieQian @basher @tmeasday

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, some tweaks

lib/api/src/modules/refs.ts Outdated Show resolved Hide resolved
lib/api/src/modules/refs.ts Outdated Show resolved Hide resolved
ndelangen and others added 5 commits August 19, 2020 13:28
…bookjs/storybook into tech/refs-verify-existence-in-node

# Conflicts:
#	lib/api/src/modules/refs.ts
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@shilman shilman changed the title Tech/refs verify existence in node Composition: Verify refs in node Aug 20, 2020
@shilman shilman added this to the 6.0.x milestone Aug 20, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever!

@stof stof deleted the tech/refs-verify-existence-in-node branch May 25, 2022 09:34
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.

4 participants