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.

Sign up for GitHub

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

Jump to bottom

use-sync-external-store: Add exports field to package.json #25231

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

use-sync-external-store: Add exports field to package.json #25231

wants to merge 1 commit into from
+12 −1

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Sep 10, 2022 •
edited
Loading

Summary

Alternate to #24440. I needed a CI build to test the published package

How did you test this change?

  • CI
  • Testing all entrypoints https://github.com/eps1lon/react-pr-25231-uses-exports-field-test 
    import A from "use-sync-external-store"
import B from "use-sync-external-store/with-selector"
import C from "use-sync-external-store/shim"
import D from "use-sync-external-store/shim/index.native"
import E from "use-sync-external-store/shim/with-selector"
import F from "use-sync-external-store/package.json" assert { type: "json" };

@sizebot
Copy link

sizebot commented Sep 10, 2022 •
edited
Loading

Comparing: 1e3e30d...360fe3d

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 153.65 kB 153.65 kB = 48.90 kB 48.90 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 155.57 kB 155.57 kB = 49.51 kB 49.51 kB
facebook-www/ReactDOM-prod.classic.js = 530.46 kB 530.46 kB = 94.67 kB 94.67 kB
facebook-www/ReactDOM-prod.modern.js = 515.71 kB 515.71 kB = 92.49 kB 92.49 kB
facebook-www/ReactDOMForked-prod.classic.js = 530.46 kB 530.46 kB = 94.67 kB 94.67 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 360fe3d

@eps1lon eps1lon changed the title use-sync-external-store: Add exports field to package.json use-sync-external-store: Add exports field to package.json Sep 10, 2022
eps1lon
eps1lon commented Sep 10, 2022
View reviewed changes
packages/use-sync-external-store/package.json
"react-native": "./shim/index.native.js",
"default": "./shim/index.js"
},
"./shim/index.native": "./shim/index.native.js",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For compat with bundlers that don't support the react-native condition. Though I don't know if we need it. I just copied it from #24440

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/facebook/react/blob/main/packages/use-sync-external-store/src/__tests__/useSyncExternalStoreNative-test.js#L45
It seems that this entry has been used in several places. I'm not sure if this is necessary.

@eps1lon eps1lon marked this pull request as ready for review September 10, 2022 12:56
@eps1lon eps1lon mentioned this pull request Sep 10, 2022
Closed
@TkDodo TkDodo mentioned this pull request Sep 12, 2022
Closed
Andarist
Andarist reviewed Sep 13, 2022
View reviewed changes
packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js
@@ -9,7 +9,7 @@

import * as React from 'react';
import is from 'shared/objectIs';
import {useSyncExternalStore} from 'use-sync-external-store/src/useSyncExternalStore';
import {useSyncExternalStore} from './useSyncExternalStore';
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a similar change should be done in other places to at least match the style?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What other places?

Copy link
Contributor

Choose a reason for hiding this comment

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

Like here:

react/packages/use-sync-external-store/with-selector.js

Line 12 in 62a5371

export {useSyncExternalStoreWithSelector} from 'use-sync-external-store/src/useSyncExternalStoreWithSelector';

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These are entrypoints so I'm not sure they should change. This PR is green and tested and I personally don't want make stylistic changes that invalidates all of that unless it's required to get this merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should fail IMHO since there are no "./src/*" exports in package.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

#23262

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the src exports will get removed at the publish stage, it might be a good idea to add it back for local development.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should fail IMHO since there are no "./src/*" exports in package.json.

react/packages/use-sync-external-store/with-selector.js is fed to rollup and will not be shipped with the src/ path. I would suggest checking out the repository that was used for testing to see how files look after bundling.

This was referenced Jan 6, 2023
Merged
Closed
@undoZen undoZen mentioned this pull request Jan 9, 2023
Closed
@eps1lon eps1lon mentioned this pull request Feb 25, 2023
Open
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@TkDodo
Copy link

TkDodo commented Apr 10, 2024

bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
@HansBrende HansBrende mentioned this pull request Apr 19, 2024
Open
@phryneas phryneas mentioned this pull request Jun 20, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 9, 2024

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Jul 9, 2024
@latin-1
Copy link
Contributor

latin-1 commented Jul 10, 2024

Bump

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Jul 10, 2024
@timdorr timdorr mentioned this pull request Jul 29, 2024
Closed
1 task
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 8, 2024

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Oct 8, 2024
@markerikson
Copy link
Contributor

VERY NOT STALE

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Oct 9, 2024
@HansBrende
Copy link

@eps1lon could you perhaps edit the PR description to include "Fixes #24590" so that that issue links to this PR along with the other PR it currently links to? Or is that PR preferable and this one should be closed in that case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@latin-1 latin-1 latin-1 left review comments

@Andarist Andarist Andarist left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@eps1lon @sizebot @TkDodo @latin-1 @markerikson @HansBrende @Andarist @facebook-github-bot