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

Make swr zero dependency #1024

Merged
merged 2 commits into from
Mar 10, 2021
Merged

Make swr zero dependency #1024

merged 2 commits into from
Mar 10, 2021

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Mar 10, 2021

make swr core dependent less and light weight. the deep equal won't change much, now make as a vendor of swr lib.

  • extract lite version of deep equal as lib
  • remove dependency

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 6d34fe5:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration

src/config.ts Outdated Show resolved Hide resolved
src/config.ts Outdated Show resolved Hide resolved
src/libs/deep-equal.ts Show resolved Hide resolved
@huozhi huozhi requested a review from shuding March 10, 2021 15:29
@shuding shuding merged commit 061db1d into vercel:master Mar 10, 2021
@huozhi huozhi deleted the zero-deps branch March 10, 2021 15:33
@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2021

This does more harm than good.

Because swr is a library, and not a built distribution (like a CLI), literally all this does is make the end-users' applications larger because any other deps that relied on dequal (or any shared dep) cannot do that anymore.

Dequal itself has no dependencies & has been extremely stable, so you had no motivation to do this.

This was done purely as a "dependency free" marketing tactic which is total bullshit & breeds distrust in library authors, like myself, who actually make dependency-free packages.

@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2021

While this stands, as per MIT licensing, you have to include my license within your LICENSE as it's now part of your package. This is theft otherwise.

@huozhi
Copy link
Member Author

huozhi commented Mar 30, 2021

@lukeed My apologies to the inappropriate way of using dequal here, and the misleading PR title which is not intend to marketing swr as a dependency free library. The PR purpose should be described as use dequal lite as inline module, that's it nothing else.

I'm super appreciated all the work you've done and the awesome libraries you built, feel sorry to bring such sore feeling to you because of my bad description which is definitely not anyone's purpose to harm origin author. I'll file another PR to include the license of dequal into swr, so much thanks for the reminding.

@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2021

and the misleading PR title which is not intend to marketing swr as a dependency free library.

The PR title is "Make swr zero dependency" – I'm not sure how I've been misled here.

I'll file another PR to include the license of dequal into swr, so much thanks for the reminding

Thank you for agreeing to update the LICENSE – I'll wait for that PR – but the rest of my points still stand: inlining dequal does more harm than good because swr's copy cannot be shared by/with other packages inside an application. It should be left as a dependency – because it IS a dependency.

I'm super appreciated all the work you've done and the awesome libraries you built,

Thank you for appreciating my work, but this does not align with those words. At best, this is hiding your appreciation. And this is Vercel shipping my code as their own, which it is not: https://www.diffchecker.com/UbAYQ6Ht

@shuding
Copy link
Member

shuding commented Mar 30, 2021

First of all, thank you for your great work on the dequal library Luke, and also thanks @huozhi for contributing!

This was done purely as a "dependency free" marketing tactic which is total bullshit & breeds distrust in library authors, like myself, who actually make dependency-free packages.

I have to clarify that this is incorrect, we are not marketing SWR as "dependency free" anywhere. The intention behind this change was to make the overall package install size smaller, as we're only using a small part of lib (dequal/lite). Another reason was while we're working on a website that uses native ES modules (with Skypack or other services), we thought it would be great to inline the dependencies so there will be fewer network requests, given SWR is such a tiny tool.

That said, it's still possible to cause a larger app size if there're other dependencies using dequal and it's really not a great way of optimization. So I think let's just revert this PR and include dequal back in the dependency list, what do you think @huozhi @lukeed?

@huozhi
Copy link
Member Author

huozhi commented Mar 30, 2021

Agree to revert it back according the smallest effort. Haven't found a good way to balance vendoring it and reducing node module size at the same time

shuding added a commit that referenced this pull request Mar 30, 2021
* add dequal

* fix
@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2021

we thought it would be great to inline the dependencies so there will be fewer network requests

You'd be saving 1 Skypack network request, at most. And even under this scenario, you'd be missing out on Cache: HIT from Skypack for dequal and/or forcing your users to make that network request anyway if they were using dequal elsewhere. This logic is really no different than the shared-npm-dependencies reason from above.

So I think let's just revert this PR and include dequal back in the dependency list, what do you think

Yes, that is an obvious course-correction. In general, you shouldn't be keen on copy-pasting library code into other libraries.

overall package install size smaller

Your package size does not / did not change. You just skipped the link.

as we're only using a small part of lib (dequal/lite)

It doesn't matter if you were only using dequal/lite – the module is purposefully set up that way so that you only import the code/variant that you actually want. Literally nothing else makes its way into swr or into your users' applications. It was made clear in #655 (and the originating #645 discussion) that this was an opt-in mode that was smaller overall that the default dequal import.

Finally, by inlining the dependency, you take on the risk/maintenance burden directly. I see you immediately ran into this (https://github.com/vercel/swr/pull/1027/files#diff-a1726aa39bf868ad52381ffb12c3fe184f0fd0eb6e8181563fe3a7cba3d38602R18) despite the fact that you only had 18 sLOC of source to copy-paste. Ethics aside, this should have been reason enough – and enough of a red-flag to yourselves – that inlining is not a great approach.


Edit: Removed inaccurate & unfair assumption about an unrelated project.

@shuding
Copy link
Member

shuding commented Mar 30, 2021

This change was reverted (#1085) and a new stable release (0.5.5) was published. Again, my apologizes but just want to make it clear that this was indeed unintentional.

@lukeed
Copy link
Contributor

lukeed commented Mar 30, 2021

Thank you

shuding added a commit that referenced this pull request May 3, 2021
* add dequal

* fix
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.

4 participants