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

Recommend client-side friendly modules #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dawsbot
Copy link

@dawsbot dawsbot commented Apr 15, 2020

✨ This PR does not change the library usage in any way, it's strictly bug prevention. ✨

A PR from the team at Everipedia

Replace all node-only packages with isomorphic packages

I'll begin by saying that if we had done this last week, this would have saved ~2 developer days.

  • If users accidentally included server-side modules in their client-side code, webpack will bundle in a large amount of backend-specific utilities in-order for the library to work. And in-the-end, these modified libraries still don't work properly. (node-fetch causes errors like this)

Or these:
image

  • The two packages I'm recommending instead will properly exclude the backend utilities if bundled for client-side. In addition to the bug mentioning above, this reduced our bundle at Everipdia over 2.3MB

@maoueh
Copy link
Contributor

maoueh commented Apr 15, 2020

I'm not quite sure about this one. I'm less inclined right now to merge this one. There is no unmanageable risk, just that, I don't feel it's the role of library to do that.

If users accidentally included server-side modules in their client-side code

This change would not prevent that to happen again in your project.

The library didn't had those dependencies listed before, they were completely "optional" when targeting. How does this impact the UMD build? Rollup is aware that those are Browser libs and does not include them in the final package? Maybe they are just ignored since not explicitly defined, I think that generates some warnings on the build, I need to check that.

The package is quite clear that pulling node-fetch and ws is required only in Node.js environment, devs targeting Browser should not pull those (that could be make clearer). If someone want to pull an isomorphic version, they are free to do so currently without any changes.

I'll need to think deeper about it.

@dawsbot
Copy link
Author

dawsbot commented Apr 16, 2020

Hi @maoueh thank you for the thoughtful response.

This PR does not impact your UMD build, so I don't intend for this PR to relate to #23 in any way. What this PR would accomplish is recommending the most fool-proof libraries for developers to use with this library.

In regards to

The package is quite clear that pulling node-fetch and ws is required only in Node.js environment, devs targeting Browser should not pull those (that could be make clearer)

The situation we found ourselves in last week was a production error where we quickly had to copy-paste in the sample code from this repo. It's our fault in the end for not making the necessary changes since we have a NextJS app, but nonetheless, this could be prevented in the future with the documentation changes proposed here.

My idea is that using unfetch and isomorphic-ws creates zero downsides for node users and creates massive upsides for both client-side and isomorphic consumers.

I understand if you want to close this, but I'm not seeing the downsides here. Is this change harmful to the documentation?

@dawsbot dawsbot closed this Apr 17, 2020
@dawsbot dawsbot deleted the bugfix/client-side-all-node-modules branch April 17, 2020 03:42
@dawsbot dawsbot restored the bugfix/client-side-all-node-modules branch April 17, 2020 03:43
@dawsbot dawsbot reopened this Apr 17, 2020
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.

2 participants