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

Don't bundle Apollo Client if the user specify their own client #1639

Merged
merged 1 commit into from
Jan 25, 2021

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Jan 14, 2021

Redwood bundles Apollo GraphQL, but offers the ability to specify your own GraphQL client. Unfortunately even when you use your own GraphQL client you'll still get Apollo bundled. This PR attempts to place Apollo's client outside of main, so if you do not use it it will not be bundled.

@Tobbe Tobbe marked this pull request as draft January 14, 2021 09:50
@github-actions
Copy link

github-actions bot commented Jan 14, 2021

@Tobbe Tobbe force-pushed the tobbe-redwoodapolloprovider branch from 1cdc9db to c4ecabe Compare January 14, 2021 12:33
@thedavidprice
Copy link
Contributor

@Tobbe could you please add some description/background to the original message in this PR? The goal is to make it possible for others in the community to follow along as well as provide context when drafting Release Notes.

Thanks in advance!

@peterp
Copy link
Contributor

peterp commented Jan 17, 2021

@thedavidprice Totally my fault, I offered to create an issue and I didn't do it in time. I'll update the description @Tobbe

@peterp
Copy link
Contributor

peterp commented Jan 17, 2021

@Tobbe What was blocking this PR again?

@Tobbe
Copy link
Member Author

Tobbe commented Jan 17, 2021

@Tobbe What was blocking this PR again?

Need to update the template in crwa. Looks like that's moving into this repo now, so I'll wait until that's settled, and then include that change in this PR as well

@Tobbe Tobbe changed the title New export of <RedwoodApolloProvider> Don't bundle Apollo Client if the user specify their own client Jan 17, 2021
@thedavidprice
Copy link
Contributor

@peterp @Tobbe along with this PR being merged, do you think it will be good timing to create a Doc about being able to swap/remove GraphQL Client?

@Tobbe
Copy link
Member Author

Tobbe commented Jan 18, 2021

@thedavidprice Funny that you should ask. I just got started on a draft for a blog post (on my blog) on how to do it. Obviously going to post on the forums too

@thedavidprice
Copy link
Contributor

Seems like a Doc would be super helpful too redwoodjs.com/docs/graphql-client And/or maybe this is a cookbook? I guess it depends on whether it's a "just get to the how-to details" or more of a walk through.

Either would be 🚀

@Tobbe
Copy link
Member Author

Tobbe commented Jan 18, 2021

Sneak peek

screencapture-localhost-8000-blog-2021-01-18-redwood-switch-to-another-graphql-client-2021-01-19-00_05_47

@Tobbe
Copy link
Member Author

Tobbe commented Jan 18, 2021

Seems like a Doc would be super helpful too redwoodjs.com/docs/graphql-client

Docs definitely sound like a good idea 👍

@peterp
Copy link
Contributor

peterp commented Jan 21, 2021

@Tobbe That PR has merged!

@Tobbe Tobbe force-pushed the tobbe-redwoodapolloprovider branch from c4ecabe to 2d7a8dc Compare January 22, 2021 21:01
@Tobbe Tobbe force-pushed the tobbe-redwoodapolloprovider branch from 2d7a8dc to c0f6164 Compare January 22, 2021 21:33
@Tobbe Tobbe marked this pull request as ready for review January 22, 2021 21:33
@Tobbe
Copy link
Member Author

Tobbe commented Jan 22, 2021

With @apollo/client

image

With graphql-hooks and graphql-hooks-memcache

image

Previously the bundle size got bigger when you used your own graphql client, because both the one you use, and apollo client were included. Now, as you can see, the bundle size goes down, and @apollo/client is no longer included when you decide to use another client instead.

@mojombo
Copy link
Contributor

mojombo commented Jan 22, 2021

Nice!

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Nice thanks @Tobbe!

I would love to figure out a pattern that allows "exports" to work without having to hack around TS and webpack, since we're going to start seeing this more and more.

I'll create some issues for this.

@peterp
Copy link
Contributor

peterp commented Jan 24, 2021

P.S. I wonder why "graphql" is packaged. It was my understanding that we transpile the gql syntax at build-time, but maybe the babel-plugin isn't working properly... Or my understanding is misguided.

@Tobbe
Copy link
Member Author

Tobbe commented Jan 25, 2021

P.S. I wonder why "graphql" is packaged

I included it to use its print function, which takes a gql ast and transforms it to a string. I replaced it by this function in my index.js file

const gqlToString = (gqlAst) => {
  return gqlAst.loc?.source.body
}

And now it looks like this
image

It was my understanding that we transpile the gql syntax at build-time, but maybe the babel-plugin isn't working properly... Or my understanding is misguided.

It's not transpiled. If it was my gqlToString function above wouldn't work (and wouldn't be needed)

@Tobbe Tobbe merged commit 8e5b123 into redwoodjs:main Jan 25, 2021
@thedavidprice thedavidprice added this to the next release milestone Jan 25, 2021
@thedavidprice
Copy link
Contributor

Nice! Moving forward, does this require:

  • any doc updates (I don't believe so)
  • any CRWA template or Tutorial updates (again, assuming not)
  • any upgrade instructions because of breaking change?

@Tobbe Tobbe deleted the tobbe-redwoodapolloprovider branch January 25, 2021 22:45
@Tobbe
Copy link
Member Author

Tobbe commented Jan 25, 2021

Nice! Moving forward, does this require:

  • any doc updates (I don't believe so)
  • any CRWA template or Tutorial updates (again, assuming not)
  • any upgrade instructions because of breaking change?

CRWA template update is part of this PR

image

This is also a breaking change. Whenever someone upgrades to the next release (0.24.0?) they'd have to do the update above manually.

The docs do mention <RedwoodProvider> in a few places. Those need to be updated to <RedwoodApolloProvider>. The tutorial is fine.

As for the breaking change, is it enough to just mention it in upgrade instructions? Or should I create a <RedwoodProvider> component that throws a helpful error message when/if someone tries to use it?

@thedavidprice
Copy link
Contributor

Docs See https://github.com/redwoodjs/redwoodjs.com/issues/528

Breaking I've labeled this and added to my release To Dos. Seems simple enough to explain. We should definitely focus on deprecation warnings and more graceful changes after v1.

Sound like a plan?

@thedavidprice thedavidprice added release:breaking This PR is a breaking change next release docs labels Jan 25, 2021
jtoar pushed a commit that referenced this pull request Oct 26, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [react-player](https://togithub.com/CookPete/react-player) | [`2.12.0`
->
`2.13.0`](https://renovatebot.com/diffs/npm/react-player/2.12.0/2.13.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>CookPete/react-player (react-player)</summary>

###
[`v2.13.0`](https://togithub.com/CookPete/react-player/blob/HEAD/CHANGELOG.md#v2130)

[Compare
Source](https://togithub.com/CookPete/react-player/compare/v2.12.0...v2.13.0)

- Fix
[#&#8203;1604](https://togithub.com/CookPete/react-player/issues/1604) -
FilePlayer does not work if I passed an array of urls
[`#1612`](https://togithub.com/cookpete/react-player/pull/1612)
- fix: `src` sttribute become "undefinded" if `url` is an array
[`#1648`](https://togithub.com/cookpete/react-player/pull/1648)
- Adding keepPlaying to other player types
[`#1639`](https://togithub.com/cookpete/react-player/pull/1639)
-   CI [`#1654`](https://togithub.com/cookpete/react-player/pull/1654)
- Swap out broken youtube URL
[`#1659`](https://togithub.com/cookpete/react-player/pull/1659)
- Add keepPlaying to seekTo
[`#1620`](https://togithub.com/cookpete/react-player/pull/1620)
- Added forceDisableHls option for FilePlayer
[`#1625`](https://togithub.com/cookpete/react-player/pull/1625)
- added onPlaybackQualityChange prop
[`#1636`](https://togithub.com/cookpete/react-player/pull/1636)
- Update the list of supported YouTube domains
[`#1599`](https://togithub.com/cookpete/react-player/pull/1599)
- Fix
[#&#8203;1604](https://togithub.com/CookPete/react-player/issues/1604) -
FilePlayer does not work if I passed an array of urls
([#&#8203;1612](https://togithub.com/CookPete/react-player/issues/1612))
[`#1604`](https://togithub.com/cookpete/react-player/issues/1604)
- Support Wisita URLs with query params
[`#1591`](https://togithub.com/cookpete/react-player/issues/1591)
- Support vimeo manage links
[`#1593`](https://togithub.com/cookpete/react-player/issues/1593)
- Update readme
[`90237f5`](https://togithub.com/cookpete/react-player/commit/90237f51d43fc63870b0e6d0c86f4497f97ca586)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
jtoar pushed a commit that referenced this pull request Nov 2, 2023
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [react-player](https://togithub.com/CookPete/react-player) | [`2.12.0`
->
`2.13.0`](https://renovatebot.com/diffs/npm/react-player/2.12.0/2.13.0)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/react-player/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/react-player/2.12.0/2.13.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>CookPete/react-player (react-player)</summary>

###
[`v2.13.0`](https://togithub.com/CookPete/react-player/blob/HEAD/CHANGELOG.md#v2130)

[Compare
Source](https://togithub.com/CookPete/react-player/compare/v2.12.0...v2.13.0)

- Fix
[#&#8203;1604](https://togithub.com/CookPete/react-player/issues/1604) -
FilePlayer does not work if I passed an array of urls
[`#1612`](https://togithub.com/cookpete/react-player/pull/1612)
- fix: `src` sttribute become "undefinded" if `url` is an array
[`#1648`](https://togithub.com/cookpete/react-player/pull/1648)
- Adding keepPlaying to other player types
[`#1639`](https://togithub.com/cookpete/react-player/pull/1639)
-   CI [`#1654`](https://togithub.com/cookpete/react-player/pull/1654)
- Swap out broken youtube URL
[`#1659`](https://togithub.com/cookpete/react-player/pull/1659)
- Add keepPlaying to seekTo
[`#1620`](https://togithub.com/cookpete/react-player/pull/1620)
- Added forceDisableHls option for FilePlayer
[`#1625`](https://togithub.com/cookpete/react-player/pull/1625)
- added onPlaybackQualityChange prop
[`#1636`](https://togithub.com/cookpete/react-player/pull/1636)
- Update the list of supported YouTube domains
[`#1599`](https://togithub.com/cookpete/react-player/pull/1599)
- Fix
[#&#8203;1604](https://togithub.com/CookPete/react-player/issues/1604) -
FilePlayer does not work if I passed an array of urls
([#&#8203;1612](https://togithub.com/CookPete/react-player/issues/1612))
[`#1604`](https://togithub.com/cookpete/react-player/issues/1604)
- Support Wisita URLs with query params
[`#1591`](https://togithub.com/cookpete/react-player/issues/1591)
- Support vimeo manage links
[`#1593`](https://togithub.com/cookpete/react-player/issues/1593)
- Update readme
[`90237f5`](https://togithub.com/cookpete/react-player/commit/90237f51d43fc63870b0e6d0c86f4497f97ca586)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/redwoodjs/redwood).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zMS41IiwidXBkYXRlZEluVmVyIjoiMzcuMzEuNSIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants