SvelteStack / svelte-query

Performant and powerful remote data synchronization for Svelte
https://sveltequery.vercel.app
MIT License
814 stars 31 forks source link

Move Storybook in it's own dir. #34

Closed frederikhors closed 3 years ago

frederikhors commented 3 years ago

Fixes https://github.com/SvelteStack/svelte-query/issues/24.

This is extremely important because many CI activities are blocked.

SomaticIT commented 3 years ago

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

amen-souissi commented 3 years ago

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 commented 3 years ago

~I tried. It works for me.~

UPDATE: it doesn't with npm 7.

amen-souissi commented 3 years ago

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

frederikhors commented 3 years ago

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 commented 3 years ago

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?

SomaticIT commented 3 years ago

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 commented 3 years ago

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

amen-souissi commented 3 years ago

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

frederikhors commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

You're right! Done!