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

ESM compliant bundling #3521

Merged
merged 19 commits into from
Apr 26, 2022
Merged

ESM compliant bundling #3521

merged 19 commits into from
Apr 26, 2022

Conversation

sachinraja
Copy link
Contributor

@sachinraja sachinraja commented Apr 17, 2022

closes #3513

  • add package.json "exports"
  • Top level files (exports) now just point to dist files instead of having their own package.json. Individual package.json files are not esm-compliant and this is already handled by "exports".
  • Types are now emitted into lib instead of a separate types directory. This will help support TypeScript, which will add support for "exports" in the next release.
  • upgrade and remove various packages related to bundling and building

Unfortunately this makes the process of adding a new export more difficult:

  1. add UMD global to scripts/rollup/umd-config.js
  2. add entry to "exports"
  3. add top level directory with files that re-export built files to support bundlers that do not support "exports"
  4. (not sure) add input file to tsconfig.types.json

@vercel
Copy link

vercel bot commented Apr 17, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-query ✅ Ready (Inspect) Visit Preview Apr 25, 2022 at 11:30PM (UTC)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 17, 2022

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 340f29b:

Sandbox Source
tannerlinsley/react-query: basic Configuration
tannerlinsley/react-query: basic-typescript Configuration

@sachinraja
Copy link
Contributor Author

Looks like it failed on node v12 because a dependency required at least node v14. Considering v12 is about to go EOL in 13 days, maybe it should be removed from CI?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 17, 2022

Looks like it failed on node v12 because a dependency required at least node v14. Considering v12 is about to go EOL in 13 days, maybe it should be removed from CI?

yes, we can totally do that for v4

@codecov
Copy link

codecov bot commented Apr 17, 2022

Codecov Report

Merging #3521 (340f29b) into beta (1af25c6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             beta    #3521   +/-   ##
=======================================
  Coverage   96.93%   96.93%           
=======================================
  Files          47       47           
  Lines        2381     2381           
  Branches      709      709           
=======================================
  Hits         2308     2308           
  Misses         71       71           
  Partials        2        2           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1af25c6...340f29b. Read the comment docs.

package.json Show resolved Hide resolved
@TkDodo
Copy link
Collaborator

TkDodo commented Apr 19, 2022

Thanks. Can you add some notes to the v4 migration guide about what the breaking changes are?

https://github.com/tannerlinsley/react-query/blob/7bba5a9754d93f7d4c5d3ca5d863b6140caf8228/docs/src/pages/guides/migrating-to-react-query-4.md#L6

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 19, 2022

Hmnm, I just tried out the codesandbox preview link from above:
https://codesandbox.io/s/tannerlinsley-react-query-basic-if97xi

and I got:

Error
No QueryClient set, use QueryClientProvider to set one

coming from the devtools 🤔 . Isn't that the issue we were actually trying to fix?

@sachinraja
Copy link
Contributor Author

sachinraja commented Apr 21, 2022

Well that's unfortunate and I'm really not sure why it's happening. It would help if I could examine the output but doesn't seem like that's possible on CSB. Seems like it's not an issue in devtools either, but rather an issue with the actual query client now.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 21, 2022

Seems like it's not an issue in devtools either, but rather an issue with the actual query client now.

no, it does seem to be an issue with the devtools. The devtools are a separate entry point, but they import directly from react-query.

But, whatever you just changed, it seems to be working now 🎉

@sachinraja
Copy link
Contributor Author

Awesome! It was probably that I set it up to resolve react-query to the index file instead of leaving it. Also looks like basic works, but basic-typescript doesn't (and has the error I was referring to above).

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 21, 2022

Also looks like basic works, but basic-typescript doesn't (and has the error I was referring to above).

you are right. in that example, we are using the PersistQueryClientProvider from the "react-query/persistQueryClient" plugin. That's probably the relevant difference?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 25, 2022

thanks. I'm still seeing size differences I cannot quite explain. Compared to beta:

ESM PR: dist/broadcastQueryClient-experimental.production.min.js: 3.55KB
BETA  : dist/broadcastQueryClient-experimental.production.min.js: 3.55KB
ESM PR: dist/createAsyncStoragePersister.production.min.js: 625B
BETA  : dist/createAsyncStoragePersister.production.min.js: 625B
ESM PR: dist/createWebStoragePersister.production.min.js: 674B
BETA  : dist/createWebStoragePersister.production.min.js: 674B
- ESM PR: dist/persistQueryClient.production.min.js: 2.19KB
+ BETA  : dist/persistQueryClient.production.min.js: 1.4KB
ESM PR: dist/react-query-core.production.min.js: 9.85KB
BETA  : dist/react-query-core.production.min.js: 9.85KB
ESM PR: dist/react-query-devtools.production.min.js: 13.71KB
BETA  : dist/react-query-devtools.production.min.js: 13.71KB
- ESM PR: dist/react-query.production.min.js: 11.36KB
+ BETA  : dist/react-query.production.min.js: 11.3KB

so it looks to me like some code of the that was previously in the persistQueryClient bundle has somehow moved towards the main react-query bundle 🤔
maybe I've added a wrong import there that is now resolved differently?

@sachinraja
Copy link
Contributor Author

Looks like that was caused by upgrading to the very latest version of @rollup/plugin-commonjs (published 2 days ago). I downgraded to the version before and I'm seeing the same numbers as the latest beta version published.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 26, 2022

splendid 🤩 . We can take a look at the latest rollup plugin separately. would be curious to know what is changing here :)

@TkDodo TkDodo merged commit 3fabd41 into TanStack:beta Apr 26, 2022
@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 4.0.0-beta.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sachinraja sachinraja deleted the fix-bundling branch April 26, 2022 11:15
@latin-1
Copy link
Contributor

latin-1 commented Apr 26, 2022

Module not found: Error: Can't resolve 'use-sync-external-store/shim' in '/home/runner/work/mono/mono/node_modules/react-query/lib/reactjs'
Did you mean 'index.js'?
BREAKING CHANGE: The request 'use-sync-external-store/shim' failed to resolve only because it was resolved as fully specified

Since use-sync-external-store doesn't include the exports field in its package.json, we should use use-sync-external-store/shim/index.js instead.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 26, 2022

@latin-1 when are you getting this error?

@sachinraja can you have a look please?

@latin-1
Copy link
Contributor

latin-1 commented Apr 26, 2022

@TkDodo I got that error from Webpack. In Webpack 5, fullySpecified is enabled by default.
https://webpack.js.org/configuration/module/#resolvefullyspecified

@sachinraja
Copy link
Contributor Author

sachinraja commented Apr 26, 2022

I don't have time to fix it right now, but @latin-1 is correct. We should change all instances of importing from use-sync-external-store/shim to use-sync-external-store/shim/index.js either through a Babel plugin or the actual source code.

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 26, 2022

@latin-1 can you contribute that maybe ?

@sachinraja
Copy link
Contributor Author

sachinraja commented Jul 18, 2022

@TkDodo I'm not sure exactly what happened but it seems like this was (unintentionally?) reverted when the project was converted to a monorepo. The package no longer has (standards-compliant) ESM support and this note in the migration docs is not true.

@TkDodo
Copy link
Collaborator

TkDodo commented Jul 19, 2022

@sachinraja yes, seems so. Can you fix it please?

@TkDodo
Copy link
Collaborator

TkDodo commented Jul 19, 2022

@sachinraja I'm pretty sure this is also stopping me from fixing this issue:

I tried to make the exports conditional, but it doesn't work for the esm builds. I remember you changed something there as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

react-query/devtools's module entry points to a CJS file
4 participants