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
Comments
Thanks for taking the time to investigate this issue. Regarding:
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. |
This is going to be a tough one in the context of this library... Splitting into Then there are libraries that have different logic between the server and the client - 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 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. |
@Izhaki Thanks for the info and code diff. I agree that breaking out the server code into Looking at your diff, couldn't we achieve this by keeping Webpack?
The benefits of this approach: maintain JS compatibility, allow us to use Webpack ecosystem tools, and reduce the build scripting required. Thoughts? |
I'm also curious if this fixes #236. If you have a chance to try it on your fork, please let me know. |
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 TerminologyTwo types of artefacts (with relation to React and React apps):
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 WebpackWebpack 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:
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 |
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 @Izhaki Yes, feel free to open any other issues. I'll plan to put together a v1 roadmap issue. |
Closing this, given that #163 is resolved. |
Is your feature request related to a problem? Please describe.
Describe the solution you'd like and how you'd implement it
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:
NextJS removes any pure server imports from client code, so this:
will become this for the client code:
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
The text was updated successfully, but these errors were encountered: