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

Add "exports" field to use-sync-external-store's package.json #24440

Closed
wants to merge 1 commit into from
Closed

Add "exports" field to use-sync-external-store's package.json #24440

wants to merge 1 commit into from

Conversation

latin-1
Copy link
Contributor

@latin-1 latin-1 commented Apr 26, 2022

Summary

How did you test this change?

@gaearon
Copy link
Collaborator

gaearon commented Apr 26, 2022

doesn't this also need ./ and ./package.json?

@latin-1
Copy link
Contributor Author

latin-1 commented Apr 26, 2022

Oops, it appears I overlooked something. Updated.

@latin-1
Copy link
Contributor Author

latin-1 commented Apr 26, 2022

I'm not sure if "./src/*": "./src/*" is necessary for this package. Should I include that in the package.json as well?

@acdlite
Copy link
Collaborator

acdlite commented Apr 26, 2022

What's the motivation for this?

@sachinraja
Copy link

This would help support React Native and be esm compliant. In TanStack/query#3582 RN support was broken because we imported from use-sync-external-store/shim/index.js to support esm which does not support directory imports and must use extensions. With this PR we can import from use-sync-external-store/shim which would work correctly for esm and RN.

@TkDodo
Copy link

TkDodo commented May 10, 2022

@gaearon @acdlite I think this PR would fix our issues we have over in react-query with the uSES shim for react native. apollo-client is having the same issue, and they have opted to copy over the uSES code to fix it on their side. I would rather wait for an official fix on your end, which I think this PR provides :)

"exports": {
".": "./index.js",
"./with-selector": "./with-selector.js",
"./shim": "./shim/index.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

thoughts on

Suggested change
"./shim": "./shim/index.js",
"./shim": {
"react-native": "./shim/index.native.js",
"default": "./shim/index.js"
},

?

That way all code can just be use-sync-external-store/shim.

note that this relies on the bundler actually supporting exports and supplying react-native condition (which I'm guessing metro at least doesn't do), but it seems more future-proof than these (seemingly) somewhat random exports

Copy link
Contributor Author

Choose a reason for hiding this comment

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

facebook/metro#670
Metro doesn't respect exports field at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, so no issue.

This stanza should still work for any modern bundler, and metro whenever it enters the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's Repack to consider (basically Webpack for react-native) so we might as well get this in now. Doing it in the future might be considered another breaking change.

@latin-1
Copy link
Contributor Author

latin-1 commented May 10, 2022

I had to say this would be a breaking change, though. That's why I held this PR. I'm trying to find out a better approach but with no luck.

@gaearon
Copy link
Collaborator

gaearon commented May 27, 2022

@latin-1 What makes this a breaking change?

@sachinraja
Copy link

sachinraja commented May 27, 2022

@gaearon You can no longer import with extensions and some paths have changed (for ESM resolution):

use-sync-external-store/index.js -> use-sync-external-store
use-sync-external-store/shim/index.js -> use-sync-external-store/shim
use-sync-external-store/with-selector.js -> use-sync-external-store/with-selector

We may be able to support it by just exporting everything under the same path as before (maybe using glob exports).

@TkDodo
Copy link

TkDodo commented Sep 9, 2022

@sachinraja @gaearon @latin-1 what's the status of this PR? If the change are okay, could we release it as a new major version? We are still having troubles with the uSES shim in react-native because of it. Thanks 🙏

@eps1lon
Copy link
Collaborator

eps1lon commented Sep 10, 2022

We are still having troubles with the uSES shim in react-native because of it. Thanks pray

Don't we need #24440 (comment) for that?

"./shim": "./shim/index.js",
"./shim/index.native": "./shim/index.native.js",
"./shim/with-selector": "./shim/with-selector.js",
"./src/*": "./src/*",
Copy link
Collaborator

@eps1lon eps1lon Sep 10, 2022

Choose a reason for hiding this comment

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

Wouldn't this require changing the publish script? The latest version of uSES publishes no src folder (see https://unpkg.com/browse/[email protected]/). What are you currently importing with use-sync-external-store/src/??

Copy link
Contributor Author

@latin-1 latin-1 Sep 11, 2022

Choose a reason for hiding this comment

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

It seems that the react package has this line.

"./src/*": "./src/*"

@@ -9,4 +9,4 @@

'use strict';

export {useSyncExternalStore} from './src/useSyncExternalStore';
Copy link
Collaborator

@eps1lon eps1lon Sep 10, 2022

Choose a reason for hiding this comment

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

This file is used internally. The published entrypoints are in npm/ which can continue using relative imports.

@eps1lon
Copy link
Collaborator

eps1lon commented Sep 10, 2022

Alternate that's not exposing non-existent src and tested: #25231

@latin-1
Copy link
Contributor Author

latin-1 commented Sep 11, 2022

Closing in favor of #25231

@latin-1 latin-1 closed this Sep 11, 2022
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.

8 participants