@storybook/react - support airbnb-js-shims v1 or v2?

[ljharb]

It'd be great if @storybook/react could allow ^1 || ^2, for deduping of either.

[ljharb]

This is still important.

[ljharb]

Closing “stale” issues is user-hostile; this remains important.

[igor-dv]

I feel that airbnb-js-shims is too minor to have it as a peer dep. We already have IMO too much peer deps, and people are going crazy on them.

What is the problem with this?

[ljharb]

I’d suggest telling your users to use install-peerdeps, at which point they won’t have to care.

Either way, while it is a peer dep, this issue is asking you to widen the semver range to ^1 || ^2.

If you want to make it both a dep and a peerDep, that should also ease users problems with it while still ensuring it’s not duplicated in the graph.

[igor-dv]

Currently, it's not a peer (at least in v4). It's a dependency of @storybook/core.

@Hypnosphi WDYT?

[ljharb]

Then by making it use the semver range I’ve asked for, Airbnb can use either version, and it will be deduped out of storybook/react - there’s still a benefit.

[igor-dv]

Yeah, I understand that making it a peer will allow us to extend the range. But I still want to get feedback of other maintainers about making it a peer.

Technically, every dependency that we have, can conflict with users setup 🤷‍♂️ we can't make every dependency to be a peer, right?

So, maybe we can solve your problem somehow else? If you are having two versions of this package in a graph, maybe you just can replace the implementation with webpack and that's all?

[ljharb]

Sorry for the confusion - you don’t need to make it a peer dep.

If you merely make the dep be ^1 || ^2 that will solve my use case.

[igor-dv]

I can't understand how it will solve it (sry for being picky).
If we will add range to the strong dep it will just install the latest one for you. What am I missing?

[igor-dv]

I've checked it locally with this structure:

npm list gives me this:

[Hypnosphi]

Agree with @igor-dv

I’d suggest telling your users to use install-peerdeps

Telling the users to use some additional tool just to make things work doesn't sound like a good idea to me

If we will add range to the strong dep it will just install the latest one for you. What am I missing?

@ljharb is probably going to rely on npm/yarn deduping. Namely, that it will pick the same version as they have in their app

[Hypnosphi]

I see two other options there:

1. Airbnb adds resolve.alias in their custom webpack config to ensure that shims are resolved to the version they depend on
2. We remove polyfills.js in 4.0 and add a note in MIGRATION.md that users now should add the polyfills they need themselves. After all, they have to do it for their own app anyway.

[ljharb]

if the range allows v1, and v1 is already present in the tree, then npm won’t install v2. No webpack is required.

Since the mention of peer deps was off topic and a misunderstanding, the (good) advice re install-peerdeps isn’t related to this thread.

[Hypnosphi]

if the range allows v1, and v1 is already present in the tree, then npm won’t install v2. No webpack is required

Looks like @igor-dv provided a counter-example: #3955 (comment)

Anyway, even if it was like you say, it would be an optimisation not a contract you could rely on

[ljharb]

That’s not how it works in practice, if you have a lockfile and v1 is at the top of the tree, and then you install something that depends on “v1 || v2” then it will not install v2.

It’s quite reliable; i rely on it with many packages. It’s how npm and semver work ¯\_(ツ)_/¯

[Hypnosphi]

I rely on it with many packages.

You're a lucky man then. That doesn't make current npm behavior in that aspect a contract.

My advice is to use option 1 from #3955 (comment). It will provide a guarantee.
Here's documentation for custom webpack configs:
https://storybook.js.org/configurations/custom-webpack-config/

[ljharb]

We need it across not just webpack, since the shims are also used in node.

Its a contract npm provides since its inception. It’s a shame you don’t want to add 4 characters and no behavior change to “dependencies” to unblock Airbnb from continuing to use storybook.

[igor-dv]

@Hypnosphi since it won't affect us, let's add this to help Airbnb friends ☺

[igor-dv]

@ljharb, BTW, if you could point us to any example or a blog post about this, that'd be great. Maybe it will solve us something as well

[ljharb]

I’m not aware of any specifically; it’s just the way npm handles its dependency graph. Shims are a unique category because they’re usually only dependencies of apps, but due to storybook’s iframes, they’re needed in this package - as such, something that would normally always be a singleton by virtue of only being defined in the top level package.json isn’t, and widening the dep range helps avoid this.

Conceptually it should really be a peerDep, since it needs to be a singleton - but that causes the UX issues mentioned above, unless it’s both a peerDep and a dep. Either way, the breaking change for the shims from v1 to v2 aren’t relevant to storybook, so “^1 || ^2” is the conceptually correct range.

[Hypnosphi]

Its a contract npm provides since its inception

Npm doesn't have lockfiles since its inception. And as demonstrated in #3955 (comment), it doesn't work like that without a pre-generated lockfile (a specific order of installation is probably also required).

What's wrong with using webpack's resolve.alias as a more explicit way to provide your own version of package?

[Hypnosphi]

@igor-dv I'd prefer not to help anyone to shoot in their feet.

[Hypnosphi]

Why is this change so objectionable?

Because the semantics of it is "let npm install either v1 or v2 depending of what its optimization algorithms find reasonable". This is not exactly what you, or anyone else, need.

[Hypnosphi]

The custom config you need may look like this:

// .storybook/webpack.config.js

module.exports = {
  resolve: {
    alias: {
      'airbnb-js-shims': require.resolve('airbnb-js-shims')
    }
  }
}

I'll really appreciate if you actually try it and tell us if anything goes wrong with it.

[ljharb]

@Hypnosphi this is exactly what I need, and I maintain what everyone needs. Whether they have v1 or v2 won't impact storybook's code, but it will impact their application's code.

The issue is that i need storybook to use v1, because my application's code uses v1. Later, when I upgrade to v2, I'll need storybook to use that. I'm not clear on how storybook's webpack config can address that, but even if that would allow me to force v1 into storybook, i'd still have an unnecessary copy of v2 installed and on disk.

[Hypnosphi]

this is exactly what I need

What you need is to use everywhere the version that you directly depend on (please correct me if I'm wrong). The resolve.alias solution gives you exactly that, thanks to require.resolve part. And it's an explicit way to say "I want this package to be a singleton". I kindly ask you to actually try it.

If 64 kilobytes of disk space becomes a real issue, we can consider option 2, that is removing the polyfills and telling the users that they should provide those themselves.

[ljharb]

64k of disk space is far more of an issue than 5 characters in a semver range that 99% of users will never interact with.

[Hypnosphi]

I never sad the characters are a problem

[ljharb]

Then i still don’t understand why you aren’t willing to widen the bottom end of the semver range for this dependency :-/

[Hypnosphi]

Because it isn't guaranteed to solve your issue. It most probably will, but it could break then with any npm patch release.

[ljharb]

It’s guaranteed to solve my issue, in my setup, with a lockfile. It wouldn’t break unless you then raised the lower bound, which you’d have no reason to do, but that’s a risk I’m happy to accept.

[Hypnosphi]

OK please open a PR. Someone will definitely approve it. Sorry for being too precaucious

[ljharb]

I’ll try! Now that master is the v4 alpha, to where should i make a PR to get this in v3?

[shilman]

@ljharb Just PR to master and we'll cherry-pick it into v3. 😄 Happy to do a release today once it's good to go.

[ljharb]

@shilman in master, it's in storybook/core, in v3, it's in storybook/react, but sure, np. (this is why i filed an issue in the first place instead of just opening a PR; because having master be an unreleased version makes contributing very confusing)

[ljharb]

Filed #4189 for v4.

[shilman]

@ljharb I see--the 3.4 release branch is release/3.4. Feel free to issue another PR there or I can handle it. Will get on this this afternoon.

[ljharb]

@shilman thanks, i'll open up that PR as well.

[ljharb]

@shilman #4190

[ljharb]

Both PRs are now merged, thanks!!

[shilman]

@ljharb Here's the first release: https://github.com/storybooks/storybook/releases/tag/v3.4.11

Let me know if that works for you, and I'll get the other one out with the other alpha changes. Fingers crossed!

[ljharb]

@shilman thanks, this worked perfectly and allowed the deduping we needed :-)