-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(react-query-devtools): only export devtools in development mode #3861
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 b0ee7f3:
|
@@ -1 +1,12 @@ | |||
export * from './devtools' | |||
if (process.env.NODE_ENV !== 'development') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it's better to do process.env.NODE_ENV === 'production'
since it matches with the behaviour of most tools (e.g. npm recognizes production
) etc. and also what tools usually recommend to set for production (e.g. webpack). While most do set development
in case it's not production, in case someone sets it to a different one (say for building with staging API etc.) then it'll cause different behaviour between other tools and React Query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can do that, I just got this from the v3 implementation. Problem is that this doesn't really work with ESM. If we take a look at the codesandbox build:
we can see the error:
Could not find module in path: '@tanstack/react-query-devtools/build/esm/devtools' relative to '/node_modules/@tanstack/react-query-devtools/build/esm/index.js'
I think the cjs builds are fine, and in the umd builds, I can see that it is properly there for dev builds and removed for prod builds. But I don't think this is how it can work with esm builds. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TkDodo afaik with webpack, we should be able to do something like this, in the package.json of devtools:
"exports": {
"production": "./index-without-devtools.js",
"default": "./index-with-devtools.js"
}
https://webpack.js.org/guides/package-exports/
I haven't tried it, but looks like it would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, we used to have this in the v4 beta, but it got lost during the monorepo move. let me see if I can restore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per usual, esm 💩 -ing on the party. Let's try our best on the esm, but if we need to ship a version with the others fixed while we figure it out, we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sachinraja fyi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to ship empty devtools
revert the node_env check
revert revert the node_env check
Codecov Report
@@ Coverage Diff @@
## main #3861 +/- ##
==========================================
+ Coverage 96.36% 96.81% +0.45%
==========================================
Files 45 57 +12
Lines 2281 2668 +387
Branches 640 784 +144
==========================================
+ Hits 2198 2583 +385
- Misses 80 83 +3
+ Partials 3 2 -1 Continue to review full report at Codecov.
|
remove outdated files
return null | ||
} as any | ||
|
||
if (process.env.NODE_ENV !== 'production') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this will cause issues when the environment is 'test'.
Perhaps something like this?
!['production', 'test'].includes(process.env.NODE_ENV)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MattyBalaam what kind of issues 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!['production', 'test'].includes(process.env.NODE_ENV)
may not work since minifier needs simple logic to do DCE. process.env.NODE_ENV !== 'production' || process.env.NODE_ENV !== 'test'
may work but it's usually better to bundle in production
mode when running e2e tests to avoid development related stuff in test.
Many other tools use process.env.NODE_ENV !== 'production'
(e.g. redux: https://github.com/reduxjs/redux-devtools/blob/a8883f287de2e36a12a4058635f8474cda34a0d4/packages/redux-devtools-extension/src/logOnlyInProduction.ts#L22) so a custom env will leave mix of dev and production code in the bundle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will also probably slow down every single test run too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the latest attempt doesn't use an env check at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I think I got it now - added the !== development env check again for commonjs builds, and esm builds now use the separate exports
field in package.json
remove main field from package.json
add env check for cjs builds
fixes #3860