-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: React-Native package exports #4887
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
base: master
Are you sure you want to change the base?
fix: React-Native package exports #4887
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
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. |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
49c76b3
to
2cecb47
Compare
@aryaemami59 can you summarize the package changes and why they improve things? |
2cecb47
to
25f7b50
Compare
25f7b50
to
c67da92
Compare
c67da92
to
2ebf48c
Compare
2ebf48c
to
c944724
Compare
9de562c
to
f9f283d
Compare
This fix is essential, Because in the latest version of Expo (Expo 53); It logs this warning uncountable times. I hope it is reviewed and merged soon |
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.
Not sure about the tsup changes (was this just a port to TS or bigger changes?) but the exports
field changes generally seem reasonable. This should at least be a minor bump, though.
"types": "./../dist/query/index.d.ts", | ||
"exports": { |
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.
Dropping the exports
field should be okay - a bundler that supports exports
should look at the root-level package.json
.
packages/toolkit/query/package.json
Outdated
@@ -4,16 +4,9 @@ | |||
"description": "", | |||
"type": "module", | |||
"module": "../dist/query/rtk-query.legacy-esm.js", | |||
"main": "../dist/query/cjs/index.js", | |||
"main": "./../dist/query/rtk-query.modern.mjs", |
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.
Here be dragons - this switches from CJS to ESM. Probably be fine, since only outdated bundlers would look at this file at all, but pointing it out.
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.
Without this vitest
will fail unless we explicitly specify resolve.mainFields
in vitest
config files. Still not entirely sure why this happens, I know it's vite
related and with this change added everything seems to work okay.
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.
On the flip side, this was here to keep Webpack 4 working.
Obviously at this point we shouldn't want to care about Webpack 4 much, but that's why it's here.
Why is Vitest looking at this field at all anyway? Shouldn't it be following exports
instead and not even looking at this file?
f9f283d
to
e4b5479
Compare
packages/toolkit/query/package.json
Outdated
@@ -4,16 +4,9 @@ | |||
"description": "", | |||
"type": "module", | |||
"module": "../dist/query/rtk-query.legacy-esm.js", | |||
"main": "../dist/query/cjs/index.js", | |||
"main": "./../dist/query/rtk-query.modern.mjs", |
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.
❓ Why did this change? main
should point to a CJS file as far as I know.
packages/toolkit/query/package.json
Outdated
@@ -4,16 +4,9 @@ | |||
"description": "", | |||
"type": "module", | |||
"module": "../dist/query/rtk-query.legacy-esm.js", | |||
"main": "../dist/query/cjs/index.js", | |||
"main": "./../dist/query/rtk-query.modern.mjs", |
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.
On the flip side, this was here to keep Webpack 4 working.
Obviously at this point we shouldn't want to care about Webpack 4 much, but that's why it's here.
Why is Vitest looking at this field at all anyway? Shouldn't it be following exports
instead and not even looking at this file?
- This is done to avoid the `module-sync` condition.
e4b5479
to
595db02
Compare
Spent a couple hours poking at this and concluded that Vitest does not seem to be looking at I honestly still don't know why. This is the closest I could find: but that just says "it uses the Node resolution algorithm", which ought to still involve As a result, we were seeing a module mismatch - it was importing What we've ended up on is specifying |
d96d5cd
to
7c1eacc
Compare
7c1eacc
to
17fed4b
Compare
Further updates: After some discussion on Bluesky, I wondered if this was specific to either Yarn or NPM in general, due to After further I've confirmed (locally at least) that this does indeed result in Vitest loading the |
This PR: