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

Implement a Svelte StoryIndexer #69

Merged
merged 4 commits into from
Oct 27, 2022
Merged

Implement a Svelte StoryIndexer #69

merged 4 commits into from
Oct 27, 2022

Conversation

j3rem1e
Copy link
Contributor

@j3rem1e j3rem1e commented Sep 4, 2022

Upgrade to Storybook v7 (alpha) and webpack5
Implement a new StoryIndexer in order to use the new "stories.json" format.

Fix #65

Two bugs, but I am not sure it come from this implementation :

  • Story background is not defined, it must be set by hand in the toolbar (probably a regression from alpha 29, or an error in the upgrade of the dependencies ?)
  • Some stories are not rendered in the Doc panel (and some are..). I didn't check in the official svelte kitchen sink if it come from the alpha29
📦 Published PR as canary version: 2.0.9--canary.69.9b95d2b.0

✨ Test out this PR locally via:

npm install @storybook/[email protected]
# or 
yarn add @storybook/[email protected]

@j3rem1e j3rem1e requested review from shilman and IanVS September 4, 2022 18:24
@socket-security
Copy link

socket-security bot commented Sep 4, 2022

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package Script field Location
[email protected] (added) postinstall package.json via @storybook/[email protected], @storybook/[email protected]
😵‍💫 Bin script confusion

This package has multiple bin scripts with the same name. This can cause non-deterministic behavior when installing or could be a sign of a supply chain attack.

Package Bin script Location
@storybook/[email protected] (added) sb package.json via [email protected]
[email protected] (added) sb package.json
Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ⚠️ 1 new install script detected
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ✅ no new native modules
Bin Script Confusion ⚠️ 2 new bin script confusions detected
Bin script shell injection ✅ no new bin script shell injection
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] [email protected]

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

@IanVS
Copy link
Member

IanVS commented Sep 29, 2022

@j3rem1e what's the best way for us to test this out? Would you be willing to release it in an alpha version, so that we can experiment with it with recent 7.0 alphas and with the svelte-vite framework?

@j3rem1e
Copy link
Contributor Author

j3rem1e commented Sep 29, 2022

It has been published with version "2.0.8--canary.8172dc5.0". We can maybe create a "future" branch, or merge it on the current branch (it should work if I remove TS definition, which doesn't exists on main)

@IanVS
Copy link
Member

IanVS commented Sep 29, 2022

cool, I'll try out the canary, I didn't see that, thanks.

@gbkwiatt
Copy link

Is there any progress on that ? It seems to work , but now there is newer canaries versions of 2.0.9 that can't be really tested without this indexer fix

Copy link
Collaborator

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

I just tested it out with v7.0.0-alpha.41 and everything seems to work perfectly.
I took the liberty to push a few fixes.

LGTM! 💪

@JReinhold JReinhold merged commit 29e1060 into main Oct 27, 2022
@JReinhold JReinhold deleted the pr/stories-json branch October 27, 2022 14:00
@shilman
Copy link
Member

shilman commented Oct 27, 2022

🚀 PR was released in v2.0.9 🚀

@shilman shilman added the released This issue/pull request has been released. label Oct 27, 2022
@j3rem1e
Copy link
Contributor Author

j3rem1e commented Oct 27, 2022

You have merged an upgrade to v7.0.0-alpha in a minor version ?
this will break any projects using this addon, or I am wrong ?

@IanVS
Copy link
Member

IanVS commented Oct 27, 2022

It looks like this was not tagged correctly before it was merged. @JReinhold maybe you could move this to a next branch, publish it as 3.0.0-alpha.1, revert this commit from main, and publish 2.0.10.

@JReinhold
Copy link
Collaborator

sorry, this was a complete blunder from me.

you are both right. I'll try to fix it.

@JReinhold JReinhold restored the pr/stories-json branch October 27, 2022 14:22
This was referenced Oct 27, 2022
@JReinhold
Copy link
Collaborator

It's fixed now with #76 (v.2.0.10) and #77 (v3.0.0-next.0).

@IanVS
Copy link
Member

IanVS commented Oct 27, 2022

Thanks for the catch, @j3rem1e, and thanks for the fixup, @JReinhold!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add indexer support
5 participants