Skip to content
This repository has been archived by the owner on Jan 20, 2022. It is now read-only.

AP-700 Export emotionCacheProviderFactory so allow consumers to override classes #46

Merged
merged 2 commits into from
Aug 11, 2019
Merged

AP-700 Export emotionCacheProviderFactory so allow consumers to override classes #46

merged 2 commits into from
Aug 11, 2019

Conversation

justinanastos
Copy link
Contributor

@justinanastos justinanastos commented Aug 5, 2019 •
edited
Loading

Ideally we'd want to use <CacheProvder> in the consumer application just like we'd want to use <ApolloProvider>, but a bug in emotion prevents this: emotion-js/emotion#1386

This provides a emotionCacheProviderFactory factory that consumers can use to create a CacheProvider.

Contributes to AP-700

@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch from 1bfb81a to a235d61 Compare August 5, 2019 19:45
@trevorblades
Copy link
Contributor

In the issue that you linked, they're talking about the issue potentially being caused by multiple instances of emotion being loaded. I noticed that this library has @emotion/core as a dependency. Maybe it should be listed as a peer dependency instead, so that the consumer is responsible for installing only one version of emotion.

I remember seeing similar issues when a library was published with react as a dependency rather than a peer dependency. What do you think? Could that be the case here?

@timbotnik
Copy link
Contributor

In the issue that you linked, they're talking about the issue potentially being caused by multiple instances of emotion being loaded. I noticed that this library has @emotion/core as a dependency. Maybe it should be listed as a peer dependency instead, so that the consumer is responsible for installing only one version of emotion.

I remember seeing similar issues when a library was published with react as a dependency rather than a peer dependency. What do you think? Could that be the case here?

I was thinking something similar, but happy to also go with this solution.

@cheapsteak
Copy link
Member

Hope that works
If it doesn't, would it make sense to try exporting a <SpaceKitProvider> that for the moment is just a Cache Provider?

@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch 2 times, most recently from 894255a to 2c3f779 Compare August 6, 2019 15:43
@justinanastos justinanastos added the minor Increment the minor version when merged label Aug 6, 2019
@justinanastos
Copy link
Contributor Author

I tried using a peer dependency and found that the issue persisted. If someone else wants to take a look and prove me wrong, I'd very much appreciate it.

@cheapsteak
Copy link
Member

Exporting the provider from space kit and importing it in engine front end "worked" for me

I was a bit surprised that it was injecting style tags into the container (style tags into the style tag, rather than style rules into the style tag). It does seem to be working as advertised (my mistake with the assumption). I think we can work with that. Will fiddle with it a bit in the morning

@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch from 82bb868 to 9060aea Compare August 6, 2019 22:35
@cheapsteak
Copy link
Member

cheapsteak commented Aug 7, 2019 •
edited
Loading

Exporting the provider and importing it in Engine Frontend does work

I debugged by replacing the entirety of AppLayout's return value in engine-frontend with

image

(<SpaceKitProvider> is just a renamed <EmotionCacheProvider> in this PR, same contents)

But, styles are placed nested inside the style tag

image

Which feels weird. Everything works though

image
image

Will defer to you folks on which way feels less weird

(Switching spaceKitEmotionStyleContainer to a <meta> tag rather than a <style> tag also works, if that's any less weird. Same with <object> tag)

@justinanastos justinanastos removed the minor Increment the minor version when merged label Aug 7, 2019
cheapsteak
cheapsteak approved these changes Aug 7, 2019
View reviewed changes
Copy link
Member

@cheapsteak cheapsteak left a comment

Choose a reason for hiding this comment

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

@justinanastos I think we're all agreed any of the solutions are fine

I trust that you'll have taken a minute to consider the option of exporting the provider from spacekit but that one is still admittedly weird with nesting things in head

@justinanastos
Copy link
Contributor Author

I hear you on that being weird @cheapsteak ; I only went with that because that's how their docs suggest doing it. I'd much rather have a CacheProvider that's handled externally but I haven't figured out how to do that yet 🤷‍♂

@cheapsteak
Copy link
Member

@justinanastos I was referring to the other solution I found still ending up weird :)

Since it specifies a "container" rather than a position, it'd injects style tags into whatever the container is, but there aren't really any valid containers that can both hold <style> elements and live in <head>. It still works but works kinda weird

@justinanastos
Copy link
Contributor Author

Since it specifies a "container" rather than a position, it'd injects style tags into whatever the container is, but there aren't really any valid containers that can both hold <style> elements and live in <head>. It still works but works kinda weird

You're totally right @cheapsteak; it's def weird.

@justinanastos
Copy link
Contributor Author

/rebase

@github-actions github-actions bot force-pushed the justin/AP-700/cache-provider branch from 9060aea to 079c14b Compare August 7, 2019 22:11
@justinanastos justinanastos added the minor Increment the minor version when merged label Aug 7, 2019
@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch from 71d9776 to 3b55335 Compare August 7, 2019 22:53
@justinanastos justinanastos mentioned this pull request Aug 9, 2019
Merged
@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch 3 times, most recently from f4afb3c to 59e42fc Compare August 11, 2019 14:25
@justinanastos justinanastos force-pushed the justin/AP-700/cache-provider branch from 59e42fc to 790fd39 Compare August 11, 2019 14:36
@justinanastos justinanastos changed the title AP-700 Use CacheProvider for all components that use emotion AP-700 Export emotionCacheProviderFactory so allow consumers to override classes Aug 11, 2019
@justinanastos justinanastos merged commit 4ffcca9 into master Aug 11, 2019
@justinanastos justinanastos deleted the justin/AP-700/cache-provider branch August 11, 2019 14:40
@justinanastos
Copy link
Contributor Author

🚀 PR was released in v1.1.0 🚀

@justinanastos justinanastos added the released This issue/pull request has been released. label Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cheapsteak cheapsteak cheapsteak approved these changes

@trevorblades trevorblades Awaiting requested review from trevorblades

@mayakoneval mayakoneval Awaiting requested review from mayakoneval

Assignees
No one assigned
Labels
minor Increment the minor version when merged released This issue/pull request has been released.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@justinanastos @trevorblades @timbotnik @cheapsteak