Skip to content

fix/umd-build #3924

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

Merged
merged 3 commits into from
Aug 10, 2022
Merged

fix/umd-build #3924

merged 3 commits into from
Aug 10, 2022

Conversation

JohnDaly
Copy link
Contributor

@JohnDaly JohnDaly commented Jul 26, 2022

UMD builds are configured to always generate output files with the format: ${packageDir}/build/umd/index.production.js and ${packageDir}/build/umd/index.development.js.

This is a problem, because there are two sets of build configs for the dev tools package: react-query-devtools and react-query-devtools-noop. Since they are both generating output files with the same name, this causes react-query-devtools-noop to always override the output of react-query-devtools.

To fix this, I updated the umdDev and umdProd functions to use the outputFile variable to determine the name of the output file. Then I provided different outputFile values for react-query-devtools and react-query-devtools-noop

- Updating "browser" field for @tanstack/react-query-devtools
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 26, 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 d85684b:

Sandbox Source
@tanstack/query-example-react-basic Configuration
@tanstack/query-example-react-basic-typescript Configuration

@@ -12,7 +12,7 @@
},
"module": "build/esm/index.js",
"main": "build/cjs/packages/react-query-devtools/src/index.js",
"browser": "build/umd/index.production.js",
"browser": "build/umd/noop.production.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the CJS and ESM production build entrypoints are noop's, I updated this to also follow that pattern

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we now have index.development.js, index.production.js, noop.developtment.js and noop.production.js produced. I don't think that's on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thought was that build/umd/index.development.js would be distributed via CDN (to support use-cases like #3916).

I'm not entirely sure what we want to do about the "browser" field here. It seems that the convention for CJS and ESM production builds is to resolve to a noop for the dev tools package? The counter-point to this would be supporting bundlers like Webpack 4, which might not know about the "exports" field and instead use the "browser" field. When I tried to use the dev tools package on a project that used an older version of Webpack, I ran into that problem and couldn't use the dev tools.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried to use the dev tools package on a project that used an older version of Webpack, I ran into that problem and couldn't use the dev tools.

yeah me to (react-scripts v3). would be good if we could support this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested the change from d85684b with react-scripts v3 and v4. Both versions are able to load @tanstack/react-query-devtools properly with that change

@@ -194,7 +194,7 @@ function umdDev({
output: {
format: 'umd',
sourcemap: true,
file: `${packageDir}/build/umd/index.development.js`,
file: `${packageDir}/build/umd/${outputFile}.development.js`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all UMD builds were generating an output file with the index name, this change will only affect the output of the react-query-devtools package

@@ -90,7 +90,7 @@ export default function rollup(options: RollupOptions): RollupOptions[] {
name: 'react-query-devtools-noop',
packageDir: 'packages/react-query-devtools',
jsName: 'ReactQueryDevtools',
outputFile: 'react-query-devtools',
outputFile: 'noop',
Copy link
Contributor Author

@JohnDaly JohnDaly Jul 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting this to noop will allow react-query-devtools-noop to generate its own output files, and not overwrite the ones for react-query-devtools

@JohnDaly
Copy link
Contributor Author

This change should allow for a new react-query-devtools/build/umd/index.development.js to be generated, which can be used to fix #3916

@@ -12,7 +12,7 @@
},
"module": "build/esm/index.js",
"main": "build/cjs/packages/react-query-devtools/src/index.js",
"browser": "build/umd/index.production.js",
"browser": "build/umd/noop.production.js",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we now have index.development.js, index.production.js, noop.developtment.js and noop.production.js produced. I don't think that's on purpose?

@codecov-commenter
Copy link

codecov-commenter commented Jul 26, 2022

Codecov Report

Merging #3924 (d85684b) into beta (e0aad73) will decrease coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             beta    #3924      +/-   ##
==========================================
- Coverage   97.11%   96.77%   -0.34%     
==========================================
  Files          50       57       +7     
  Lines        2391     2666     +275     
  Branches      706      782      +76     
==========================================
+ Hits         2322     2580     +258     
- Misses         67       84      +17     
  Partials        2        2              
Impacted Files Coverage Δ
.../persistQueryClient/PersistQueryClientProvider.tsx
src/core/hydration.ts
src/tests/utils.ts
src/core/removable.ts
src/core/focusManager.ts
src/devtools/styledComponents.ts
src/core/query.ts
src/core/queriesObserver.ts
src/core/infiniteQueryObserver.ts
src/reactjs/useSyncExternalStore.ts
... and 97 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@TkDodo
Copy link
Collaborator

TkDodo commented Jul 28, 2022

@JohnDaly we want to release all build related things in a pre-release version. There is also this PR that re-adds proper ESM support:

both of them should go to the beta branch please, and there are likely conflicts. thanks 🙏

@TkDodo TkDodo changed the base branch from main to beta July 28, 2022 08:30
@phil-saam
Copy link

This bug is preventing me from upgrading to v4 as I currently have to use an older version of react-scripts. When do you think this will be merged?

@@ -12,7 +12,7 @@
},
"module": "build/lib/index.mjs",
"main": "build/lib/index.js",
"browser": "build/umd/index.production.js",
"browser": "build/lib/index.js",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should allow for older bundlers (e.g. Webpack 4) to properly resolve @tanstack/react-query-devtools

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that we should remove browser field from all packages.
UMD build should be linked directly from CDN.
We do not ship separate code for node and browser (ie. xxx.server.js and xxx.client.js), so we could stick to main field.
Or remap all packages this way.

@JohnDaly JohnDaly requested a review from TkDodo August 6, 2022 01:01
@TkDodo TkDodo merged commit dc8aec7 into TanStack:beta Aug 10, 2022
@JohnDaly JohnDaly deleted the fix/umd-build branch August 11, 2022 02:27
@TkDodo
Copy link
Collaborator

TkDodo commented Aug 12, 2022

@JohnDaly it seems that this PR increased the bundle-size of react-query by 5x 😅

https://bundlephobia.com/package/@tanstack/[email protected]

We can also see this in the bundlewatch logs:

packages/react-query/build/umd/index.production.js: 43.89KB (gzip)

size on main is:

packages/react-query/build/umd/index.production.js: 11.37KB (gzip)

any idea what that could be about?

@JohnDaly
Copy link
Contributor Author

JohnDaly commented Aug 12, 2022

@JohnDaly it seems that this PR increased the bundle-size of react-query by 5x 😅

https://bundlephobia.com/package/@tanstack/[email protected]

We can also see this in the bundlewatch logs:

packages/react-query/build/umd/index.production.js: 43.89KB (gzip)

size on main is:

packages/react-query/build/umd/index.production.js: 11.37KB (gzip)

any idea what that could be about?

I ran the build on the beta branch, and noticed that react-dom was being included:

Beta build

Screen Shot 2022-08-12 at 9 02 12 AM

Main build

Screen Shot 2022-08-12 at 9 02 41 AM

I applied this changes from this PR onto the code in the main branch, and it created a UMD production build that did not include react-dom:

packages/react-query/build/umd/index.production.js... 
index.production.js ⏤  11.6 kB

There are a few things that I noticed that are different between the beta branch and the main branch, that might be contributing to this:

1. The Rollup configs are different

Specifically the main branch doesn't include some of the globals that are specified in the beta branch.

Example '@tanstack/query-core': 'QueryCore' is not present in main, but is in beta:

    ...buildConfigs({
      name: 'react-query',
      packageDir: 'packages/react-query',
      jsName: 'ReactQuery',
      outputFile: 'index',
      entryFile: 'src/index.ts',
      globals: {
        react: 'React',
-       '@tanstack/query-core': 'QueryCore',
      },
    }),

2. The package.json for packages are different

There are changes to the fields that bundlers use to determine how to resolve packages. We can see that the entrypoints (e.g. module, main, exports) are quite different between branches and that the sideEffects field is as well:

Example: Partial diff for the package.json file from "@tanstack/react-query"

"name": "@tanstack/react-query",
- "module": "build/lib/index.mjs",
+ "module": "build/esm/index.js",
- "main": "build/lib/index.js",
+ "main": "build/cjs/react-query/src/index.js",
  "browser": "build/umd/index.production.js",
- "types": "build/lib/index.d.ts",
+ "types": "build/types/packages/react-query/src/index.d.ts",
- "exports": {
-   ".": {
-     "types": "./build/lib/index.d.ts",
-     "import": "./build/lib/index.mjs",
-     "default": "./build/lib/index.js"
-   },
-   "./package.json": "./package.json"
- },
  "sideEffects": [
-   "./src/setBatchUpdatesFn.ts"
+   "lib/index.js",
+   "lib/index.mjs",
+   "lib/reactjs/index.js",
+   "lib/reactjs/index.mjs",
+   "lib/reactjs/setBatchUpdatesFn.js",
+   "lib/reactjs/setBatchUpdatesFn.mjs"
  ],

@DamianOsipiuk
Copy link
Contributor

@JohnDaly Yeah basically it's due to missing global declaration of react-dom. I will make the fix for it. Tested already that it fixes the issue.

TkDodo added a commit that referenced this pull request Sep 10, 2022
* fix: restore missing `exports` declarations (#3892)

* fix: restore missing `exports` declarations

* fix: restore package.json exports[C

* fix: reexport types used by vue-query

* fix: react-native uSES usage

* fix: emit mjs for esm

* fix: uSES build

* fix: devtools exports to allow devtools in prod

* fix: cjs and esm build bundled to lib dir

* fix: sideEffect in react-query, better files paths

* fix: generate declarations to lib

* fix: lint and tests

* fix: use the same ts build method for tests

* fix: change force prod import

* fix: subpackage dependencies (#4013)

* fix: umd-build (#3924)

* - Fix UMD build getting overwritten
- Updating "browser" field for @tanstack/react-query-devtools

* Updating the "browser" field to be the same as "main"

* release: v4.0.11-beta.0

* release: v4.0.11-beta.1

* fix(react-query-devtools): cjs devtools fallback (#4048)

* release: v4.0.11-beta.2

* fix: remove browser entry, fix umd size (#4044)

* fix: remove browser entry, fix umd size

* fix: bundle query-core with react-query for umd

* fix: remove missed browser entry

* chore: remove 'browser' field from package validation

because it doesn't exist anymore

* release: v4.0.11-beta.3

* release: v4.0.11-beta.4

* chore: react-query should be a peerDependency of the devtools

* release: v4.0.11-beta.5

* release: v4.3.0-beta.0

* fix: webpack 4 fallback to cjs (#4069)

* fix: publish script shouldn't check against module anymore

* fix: publish script

I don't think we can have single quotes in commit message when passing them to --notes

* release: v4.3.0-beta.2

* release: v4.3.0-beta.3

* fix: umd build size, force prod devtools (#4074)

* fix: umd build size

* fix: devtools force production

* release: v4.3.0-beta.4

* release: v4.3.0-beta.5

* fix: reintroduce production export (#4090)

* release: v4.3.0-beta.6

* fix(react-query-devtools): always useEffect for the mounted check

no effect runs on the server, and there is no real advantage to useLayoutEffect on the client; somehow, this dynamic check creates problems with the production build of the devtools

* release: v4.3.0-beta.7

* docs: document devtools in production

* docs: document devtools in production

* docs: document devtools in production

nodenext needs 4.7

* fix: support react-native (#4125)

* fix: support react-native

* chore: remove banner from build

Co-authored-by: Damian Osipiuk <[email protected]>
Co-authored-by: John Daly <[email protected]>
Co-authored-by: Tanner Linsley <[email protected]>
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.

5 participants