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 react/internal entry point, reorganize imports #11426

Closed
wants to merge 136 commits into from

Conversation

phryneas
Copy link
Member

We had the bundling issues, because both /react and /react/hooks exported functions that relied on SuspenseCache, but there was no way of importing SuspenseCache from a common place between the two.

Let's see if this solves it somehow.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@phryneas phryneas marked this pull request as draft December 12, 2023 10:47
@phryneas
Copy link
Member Author

/release:pr

Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-11426-20231212121205.

@phryneas
Copy link
Member Author

phryneas commented Dec 12, 2023

The diffs for react/react.cjs and react/hooks/hooks.cjs look promising, but why is size-limits suddenly increased by so much?

Comparing the (prettified) apollo-client.min.cjs files doesn't show a significant difference:

% diff old.js new.js                                                                                                                                                                          pr/react-internal
25c25
<   d = "0.0.0-pr-11412-20231212060506";
---
>   d = "0.0.0-pr-11426-20231212121205";
6589c6589
<   w("createContext" in p, 46);
---
>   w("createContext" in p, 45);
6607c6607
<   return w(!!r, 50), r;
---
>   return w(!!r, 49), r;
7406c7406
<       return w(t && t.client, 45), e.children(t.client);
---
>       return w(t && t.client, 44), e.children(t.client);
7422c7422
<     return w(s.client, 47), p.createElement(i.Provider, { value: s }, n);
---
>     return w(s.client, 46), p.createElement(i.Provider, { value: s }, n);
7688c7688
<           w(!h(), 51);
---
>           w(!h(), 50);

@phryneas
Copy link
Member Author

Ah... incorrect size limits in the parent PR. Explains a lot :D

I updated them over there.

@@ -50,7 +50,7 @@ export {
makeVar,
} from "../cache/index.js";

export * from "../cache/inmemory/types.js";
export type * from "../cache/inmemory/types.js";
Copy link
Member Author

Choose a reason for hiding this comment

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

Kinda unrelated, but this came up as a "Risky cross-entry-point nested import" and we're not using it for any runtime code, so it seems like a good idea.

@phryneas phryneas marked this pull request as ready for review December 12, 2023 13:44
@phryneas phryneas requested a review from jerelmiller December 12, 2023 13:44
@@ -1,9 +1,5 @@
name: Compare Build Output
on:
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this was to check for this PR but will need to be reverted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I think we should leave it in. This check is always useful, not only against release branches, and it doesn't save us a lot of time not running it.

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Changes look good! I'll leave it up to you to decide when to merge.

"@apollo/client": patch
---

Changes to bundling to prevent duplication of shipped code
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Changes to bundling to prevent duplication of shipped code
Move Suspense cache internals to own internal entrypoint to prevent duplication of shipped code

Perhaps we could be a bit more explicit about what was changed?

Copy link
Member Author

@phryneas phryneas Dec 13, 2023

Choose a reason for hiding this comment

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

Honestly, I just added that Changeset to be able to do a /pr:release - I'll remove that one and then you can just merge this into your branch whenever you want. I don't think this needs a changeset :)

@github-actions github-actions bot added the auto-cleanup 🤖 label Dec 12, 2023
@jerelmiller
Copy link
Member

And thanks for digging into this! Glad you spotted the build oddity on my other PR before it shipped!

Base automatically changed from jerel/load-query-outside-react to release-3.9 December 18, 2023 17:07
@jerelmiller
Copy link
Member

Closing in favor of #11439

@jerelmiller jerelmiller deleted the pr/react-internal branch December 19, 2023 04:59
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants