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

Use standard and unified CommonJS/esm distribution #207

Closed
Izhaki opened this issue Jun 15, 2021 · 7 comments
Closed

Use standard and unified CommonJS/esm distribution #207

Izhaki opened this issue Jun 15, 2021 · 7 comments
Labels
breaking The issue or PR will introduce a breaking change enhancement New feature or request v1 Change intended to land in v1.x

Comments

@Izhaki
Copy link
Contributor

Izhaki commented Jun 15, 2021

  • Already done on my fork, (see changes).
  • Still need to update the docs.
  • Happy to submit a PR.
  • Breaking changes.

Is your feature request related to a problem? Please describe.

  • Currently this package is distributed using a client and server distributions.
  • This was identified as the cause for issues like withAuthUser() HOC prevents Fast Refreshing / HMR #163 (Fast Refresh not working).
  • The distributions are webpack bundles, despite the fact that this package is certain to be consumed by NextJS which uses webpack. Webpack would do a much better job handling this package and its export if these are not bundled, but rather exported in CommonJS or esm format. This will allow Tree-shaking and build time saving. In other work, this package is bundled to be distributed and then bundled again (by NextJS/Webpack).
  • The way this package is distributed is hardly conventional.

Describe the solution you'd like and how you'd implement it

  • Package should be exported using standard common JS or esm.
  • No split between client/server distributions.

Is this a breaking change?
Yes.

In order for client NextJS build not complaining about node constructs (os, path, crypto... I stopped counting at the 13th one), any server function will have to be imported from a sub folder, like so:

import { useAuthUser, withAuthUser } from 'next-firebase-auth'
import { withAuthUserTokenSSR } from 'next-firebase-auth/server'

NextJS removes any pure server imports from client code, so this:

import { useAuthUser, withAuthUser, withAuthUserTokenSSR } from 'next-firebase-auth'

will become this for the client code:

import { useAuthUser, withAuthUser } from 'next-firebase-auth'

But 'next-firebase-auth' is not tree-shaken in development builds, and as it is importing server-related modules it will still break the client build (see comment, and confirmed in local try out).

Describe alternatives you've considered
None

@Izhaki Izhaki added the enhancement New feature or request label Jun 15, 2021
@kmjennison
Copy link
Contributor

Thanks for taking the time to investigate this issue.

Regarding:

The way this package is distributed is hardly conventional

Do you have any resources/examples you can share about distributing isomorphic Node packages? Before we make this change (especially because it's breaking), I'd like to get a better idea of best practices.

@Izhaki
Copy link
Contributor Author

Izhaki commented Jun 27, 2021

Do you have any resources/examples you can share about distributing isomorphic Node packages?

This is going to be a tough one in the context of this library...

Splitting into browser and main/module is absolutely fine in many scenarios, for instance, that's what puhser does. This is one approach to "distributing isomorphic Node packages"

Then there are libraries that have different logic between the server and the client - firebase-admin and firebase/app respectively.

But none is exporting React Components. A React component should have no discrepancy between Server/Client - it's a view construct and should render the same on both (or your React app will not hydrate properly).

When there are differences between Client/Server behaviour you may see two different components, like with React Router. But do note that these components do not manipulate the (to be hydrated) view.

In the way it is distributed now, this packages makes a loose separation between the React components (including HOCs) and the auth logic.

There are multitude of ways to solve this, in the solution on my branch I simply put server stuff in the /server folder and enabled importing form that /server folder. This may not be the best strategy, but it was the low-hanging-fruit.

Seeing as this is breaking change (a major version bump), we may just as well wish to think this through, but it will mean larger-scale changes.

If you wish, and I find the time, I'm happy to write an RFC on possible improvements for the next version.

@kmjennison
Copy link
Contributor

@Izhaki Thanks for the info and code diff. I agree that breaking out the server code into next-firebase-auth/server looks like the right decision.

Looking at your diff, couldn't we achieve this by keeping Webpack?

  • Modify index.js and server/index.js like you did
  • Remove the "browser" property from package.json

The benefits of this approach: maintain JS compatibility, allow us to use Webpack ecosystem tools, and reduce the build scripting required.

Thoughts?

@kmjennison
Copy link
Contributor

I'm also curious if this fixes #236. If you have a chance to try it on your fork, please let me know.

@Izhaki
Copy link
Contributor Author

Izhaki commented Aug 3, 2021

Looking at your diff, couldn't we achieve this by keeping Webpack?

I guess we could. I think if anything Webpack is not going to be the issue here, but rather the fact that we are going to end up with two bundles and we still have an issue with Fast Refresh. So I guess the answer is - we wouldn't know until we try.

But then there's an argument why not to use Webpack and it goes like this:

Some Context and Terminology

Two types of artefacts (with relation to React and React apps):

  • Apps - something that you deploy.
  • Packages - something you publish (say to npm) and then get bundled into an app. Sometimes the name library is used, although nitpickers will distinguish between a framework (calls your code, like React) and a library (your code calls it, like lodash).

Also, I've never seen any official figures, but I bet my car that more than 95% of React apps are bundled using WebPack.

Against Webpack

Webpack is a bundler. It works a treat with apps, gives us HMR and Fast Refresh and incremental builds and whatnot.

It is used far less commonly to publish packages. This is a problem because how other packages go about transpiling the dist folder is priceless (and normally your starting point).

I guess the main reason is that packages, specifically React related packages, do not need to be distributed as bundles, on the contrary - they are better shipped as individual files.

Bundling will include some bundler-specific code (webpack runtime/manifast). Not a problem for apps.

But a React package will be bundled again into an app, so better let the app bundler do the bundling, rather than let the bundler bundle a bundle (there are some exceptions to this, but not relevant to this package).

Now consider the following distribution you may want to support:

  1. CommonJS - (main) this is the common denominator that is universally supported (node or webpack, but not directly in the browser). Having said that, it is only a must because some people still use CommonJS code in naive SSR code. In nearly all other cases the next option will be preferred.
  2. Legacy ESM - (module).
  3. Modern ESM - (exports). This is the future and is already the first choice in node 12 and Webpack 5.
  4. umd - for CDNS.

Of all these the only one that truly need to be a bundle is the umd one. For a React library why would you bundle any other type?

(see https://github.com/developit/microbundle for more)

Who Knows?

Now you could get away with just a bundled (using Webpack) CommonJS distribution.

I have spent a lot of time with Webpack, but never to publish a library. I have no idea what quirks are ahead (and there usually are), and I suspect guides/examples are scarce.

It's your call really.


PS
If already introducing breaking changes. I have an idea for a few more of them. So let me know if you wish me to spell these in an v1 RFC.

@kmjennison
Copy link
Contributor

We should stick with Webpack until we identify any specific problems of doing so. It's definitely used for libraries (Webpack docs) and React components (e.g., blog post).

As of now, this issue serves only to solve #163. If we identify another way to fix HMR and no other reasons for a next-firebase-auth/server refactor, we can close this.

@Izhaki Yes, feel free to open any other issues. I'll plan to put together a v1 roadmap issue.

@kmjennison kmjennison mentioned this issue Aug 7, 2021
24 tasks
@kmjennison kmjennison added the v1 Change intended to land in v1.x label Aug 25, 2022
@kmjennison
Copy link
Contributor

Closing this, given that #163 is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking The issue or PR will introduce a breaking change enhancement New feature or request v1 Change intended to land in v1.x
Projects
None yet
Development

No branches or pull requests

2 participants