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

Move Storybook in it's own dir. #34

Merged
merged 6 commits into from
Apr 7, 2021
Merged

Move Storybook in it's own dir. #34

merged 6 commits into from
Apr 7, 2021

Conversation

frederikhors
Copy link
Collaborator

@frederikhors frederikhors commented Apr 6, 2021

Fixes #24.

This is extremely important because many CI activities are blocked.

@SomaticIT
Copy link
Collaborator

👍
It also removes a lot of warnings during the development (anytime you update a dependency).

@amen-souissi
Copy link
Collaborator

Thanks! Yes why not :)

Firstly, you need to run the storybook and verify if it works correctly. Otherwise, you need to fix the storybook versions and generate the package-lock.json.

@frederikhors
Copy link
Collaborator Author

frederikhors commented Apr 6, 2021

I tried. It works for me.

UPDATE: it doesn't with npm 7.

@amen-souissi
Copy link
Collaborator

You need to remove the node-modules folder and try again...

@frederikhors
Copy link
Collaborator Author

Ok I'm trying with Windows 10, npm 7.8.0 and node 14.16.0.

And I'm getting this error now:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE unable to resolve dependency tree
npm ERR!
npm ERR! While resolving: @sveltestack/svelte-query@1.3.0
npm ERR! Found: rollup@2.39.1
npm ERR! node_modules/rollup
npm ERR!   dev rollup@"^2.39.1" from the root project
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer rollup@"^1.20.0" from @rollup/plugin-node-resolve@6.1.0
npm ERR! node_modules/@rollup/plugin-node-resolve
npm ERR!   dev @rollup/plugin-node-resolve@"^6.0.0" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force, or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

this error is because of this: https://stackoverflow.com/a/66035709 and Storybook's team is aware and they are fixing this.

Yarn doesn't fix the problem, it gets around it, as npm did before v7.

We can fix this moving Storybook in a separate/dedicated dir.

I think it's also better because it gets leaner for the real product (which is svelte-query), even for us who use it and don't need the storybook dependencies in each installation.

I'll update this PR with this change if you like.

@frederikhors
Copy link
Collaborator Author

frederikhors commented Apr 7, 2021

I updated the PR with those changes.

Tests are green with some warnings.

Until they (Storybook team) fix the problem with npm 7 the installation can be done with yarn or with npm i --legacy-peer-deps.

We can also remove yarn from anywhere if you want.

@SomaticIT what do you think?

@frederikhors frederikhors changed the title Remove engines field from package.json Move Storybook in i'ts own dir. Apr 7, 2021
@frederikhors frederikhors changed the title Move Storybook in i'ts own dir. Move Storybook in it's own dir. Apr 7, 2021
@SomaticIT
Copy link
Collaborator

For me the issue is not about contributing inside the project.

If you think this project needs yarn to be developped, I don't see any problem with that.
This should only be documented in the CONTRIBUTING.md (which is done).

The problem I see is when you install this lib as a dependency of another project, it prints a warning that could create confusion. It says that you need to use yarn to work with this lib but it's not true, the lib could be installed with npm with no problem.

@frederikhors
Copy link
Collaborator Author

Storybook starts now! It's done! Can you verify, @amen-souissi?

@amen-souissi
Copy link
Collaborator

Yes, it works fine with npm and yarn. Last thing ... You have to adapt the readme file and it's OK.

@frederikhors
Copy link
Collaborator Author

frederikhors commented Apr 7, 2021

I don't know what to change in README.md.

The commands are still OK.

We are not removing yarn so it's still fine like it is now:

git clone git@github.com:SvelteStack/svelte-query.git
cd svelte-query
yarn
yarn storybook

Am I wrong?

@amen-souissi
Copy link
Collaborator

For storybook, we need to install the project independently. We should add a storybook section for example.
To run storybook:
cd storybook
yarn && yarn start

@frederikhors
Copy link
Collaborator Author

You're right! Done!

@amen-souissi amen-souissi merged commit ebf184b into SvelteStack:main Apr 7, 2021
@frederikhors frederikhors deleted the patch-1 branch April 7, 2021 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why this line in package.json?
3 participants