Add warning if multiple versions are running (addresses #1470) #1677

ajs139 · 2019-12-06T19:21:30Z

What:
Add a register of all the @emotion packages, so that a warning can be displayed if the same package is registered with multiple versions.

Why:
Running multiple versions of the packages can cause hard to trace issues, see #1470.

How:
Each module "registers" itself providing its name and version into a global object. If that module name is already registered for a different version, a warning is displayed.

This is just a draft, for discussion on the approach.

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2019-12-06T19:21:33Z

🦋 Changeset is good to go

Latest commit: b29b958

We got this.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ajs139 · 2019-12-06T19:42:41Z

packages/utils/src/index.js

+registerEmotionModule(
+  require('../package.json').name,
+  require('../package.json').version
+)


This is more here for usage, it may be decided that it's OK for this module to have multiple concurrent versions.

Andarist · 2019-12-08T09:05:49Z

packages/jest-emotion/src/index.js

@@ -1,4 +1,5 @@
 // @flow
+import { registerEmotionModule } from '@emotion/utils'


i think this only matters for @emotion/core package - don't think there is a much need for this in other packages

Andarist · 2019-12-08T09:11:16Z

packages/utils/src/index.js

+
+export function registerEmotionModule(name: string, version: string) {
+  if (versions[name] && versions[name] !== version) {
+    console.warn(


It's not only about different versions (but that too) - the problem might also be caused by requiring the same version twice (from different locations at disk).

I think react-router prepared this pretty much in a way that I'd like to have this included here, take a look:
https://github.com/ReactTraining/react-router/blob/e81dfa2d01937969ee3f9b1f33c9ddd319f9e091/packages/react-router/modules/index.js#L1

For that to work exactly like in their case we'd have to have process.env.ROLLUP_MODULE_FORMAT available - this could potentially be added in our build tool (preconstruct). I expect supporting this would only require changes around those lines:
https://github.com/preconstruct/preconstruct/blob/120f9b6d747e985ed917a898f13d7177fd74ef73/packages/cli/src/build/rollup.ts#L164-L168

If we want to warn based on module type as well as version, we would want to split this up:
https://github.com/preconstruct/preconstruct/blob/c1cfc0503f73a197487d6cba6ada607fe957f527/packages/cli/src/build/config.ts#L132-L153
If we allowed the messaging to be more vague, then we wouldn't need to do an explicit check of the module type. For example, we could change the code to this:

if (__DEV__) { if (versions[name]) { if (versions[name] === version) { console.warn( `Package ${name}@${versions[name]} is already registered,` + `so things won't work right. This would only happen if you are `+ `loading two different builds of ${name}.` ); } else { console.warn( `Package ${name}@${versions[name]} is already registered.` + `Running ${version} may cause problems.` ); } } versions[name] = version; }

The goal of this is to warn about second copy of @emotion/core being loaded. It doesn't really matter what was the first version being loaded and it doesn't even matter what is the second one - we only need types of both to provide a helpful error message. I would limit this to a build type error - not sure if we need to complicate this with providing also an exact version.

codesandbox-ci · 2019-12-17T17:01:47Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b29b958:

Sandbox	Source
Emotion	Configuration

ajs139 · 2019-12-17T17:05:28Z

packages/utils/src/index.js

+const instances: ModuleRegister = getGlobalInstances(globalContext)
+
+export function registerEmotionModule(name: string) {
+  if (process.env.NODE_ENV !== 'production') {


tried using babel-plugin-dev-expression, but it was causing failures when running the test suite.

codecov · 2019-12-17T17:10:44Z

Codecov Report

Merging #1677 into next will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/core/src/index.js	`100% <100%> (ø)`
packages/utils/src/index.js	`100% <100%> (ø)`	⬆️
packages/cache/src/index.js	`98.9% <0%> (-0.03%)`	⬇️
packages/sheet/src/index.js	`100% <0%> (ø)`	⬆️

Andarist · 2019-12-18T20:33:00Z

packages/utils/src/index.js

+
+const instances: ModuleRegister = getGlobalInstances(globalContext)
+
+export function registerEmotionModule(name: string) {


I don't think this abstraction is much needed, we only need this for a single module (@emotion/core), so I would prefer avoiding extra abstractions and just inlining this stuff to @emotion/core's index

I also don't think we really need tests for this - it's rather hard to break it and writing a proper test (without testing implementation details) for this thing would be rather brittle and complicated

Andarist · 2019-12-19T21:35:15Z

packages/core/src/utils.js

+const globalKey = '__EMOTION_CORE__'
+const globalContext = isBrowser ? window : global
+
+export function checkForMultipleVersions() {


this would have to be called to actually make it work :p however, calling this as a function could lead to this not being properly removed in production, so I would just move this function's content to core/src/index.js and this should be good to go 👍

Andarist · 2019-12-21T09:54:35Z

@ajs139 the only thing missing is an appropriate changeset, could you add it?

emmatown

Other than adding a changeset, LGTM

…) (emotion-js#1677) * Add warning if multiple versions are running (addresses emotion-js#1470) * Add changeset

ajs139 force-pushed the warn_multiple_versions branch from dd191ac to 5c548f3 Compare December 6, 2019 19:23

ajs139 commented Dec 6, 2019

View reviewed changes

Andarist reviewed Dec 8, 2019

View reviewed changes

ajs139 force-pushed the warn_multiple_versions branch from 5c548f3 to 2d82de4 Compare December 17, 2019 17:00

ajs139 force-pushed the warn_multiple_versions branch from 2d82de4 to 0a2cdcc Compare December 17, 2019 17:02

ajs139 commented Dec 17, 2019

View reviewed changes

Andarist reviewed Dec 18, 2019

View reviewed changes

ajs139 force-pushed the warn_multiple_versions branch from 0a2cdcc to 9955147 Compare December 19, 2019 16:16

Andarist reviewed Dec 19, 2019

View reviewed changes

ajs139 force-pushed the warn_multiple_versions branch from 9955147 to e275b57 Compare December 20, 2019 14:00

Add warning if multiple versions are running (addresses emotion-js#1470)

1410fcb

ajs139 force-pushed the warn_multiple_versions branch from e275b57 to 1410fcb Compare December 20, 2019 14:25

Andarist approved these changes Dec 21, 2019

View reviewed changes

Andarist requested a review from emmatown December 21, 2019 09:54

emmatown approved these changes Dec 21, 2019

View reviewed changes

Add changeset

b29b958

emmatown marked this pull request as ready for review December 24, 2019 02:31

emmatown merged commit d62d910 into emotion-js:next Dec 24, 2019

bmvantunes mentioned this pull request Jan 16, 2020

[jest.mock + Typescript] You are loading @emotion/react when it is already loaded. Running multiple instances may cause problems. #1727

Closed

louisgv pushed a commit to louisgv/emotion that referenced this pull request Sep 6, 2020

Add warning if multiple versions are running (addresses emotion-js#1470…

47d70a7

…) (emotion-js#1677) * Add warning if multiple versions are running (addresses emotion-js#1470) * Add changeset

github-actions bot mentioned this pull request Nov 10, 2020

Version Packages #2046

Merged

CodingItWrong mentioned this pull request Nov 4, 2022

[question] Multiple instances of MUI and Emotion in the same page mui/material-ui#35006

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warning if multiple versions are running (addresses #1470) #1677

Add warning if multiple versions are running (addresses #1470) #1677

ajs139 commented Dec 6, 2019

changeset-bot bot commented Dec 6, 2019 •

edited

Loading

ajs139 Dec 6, 2019

Andarist Dec 8, 2019

Andarist Dec 8, 2019

ajs139 Dec 10, 2019 •

edited

Loading

Andarist Dec 14, 2019

codesandbox-ci bot commented Dec 17, 2019 •

edited

Loading

ajs139 Dec 17, 2019

codecov bot commented Dec 17, 2019 •

edited

Loading

Andarist Dec 18, 2019

Andarist Dec 19, 2019

Andarist commented Dec 21, 2019

emmatown left a comment

		@@ -1,4 +1,5 @@
		// @flow
		import { registerEmotionModule } from '@emotion/utils'


		const instances: ModuleRegister = getGlobalInstances(globalContext)

		export function registerEmotionModule(name: string) {

Add warning if multiple versions are running (addresses #1470) #1677

Add warning if multiple versions are running (addresses #1470) #1677

Conversation

ajs139 commented Dec 6, 2019

changeset-bot bot commented Dec 6, 2019 • edited Loading

🦋 Changeset is good to go

ajs139 Dec 6, 2019

Choose a reason for hiding this comment

Andarist Dec 8, 2019

Choose a reason for hiding this comment

Andarist Dec 8, 2019

Choose a reason for hiding this comment

ajs139 Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

Andarist Dec 14, 2019

Choose a reason for hiding this comment

codesandbox-ci bot commented Dec 17, 2019 • edited Loading

ajs139 Dec 17, 2019

Choose a reason for hiding this comment

codecov bot commented Dec 17, 2019 • edited Loading

Codecov Report

Andarist Dec 18, 2019

Choose a reason for hiding this comment

Andarist Dec 19, 2019

Choose a reason for hiding this comment

Andarist commented Dec 21, 2019

emmatown left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Dec 6, 2019 •

edited

Loading

ajs139 Dec 10, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 17, 2019 •

edited

Loading

codecov bot commented Dec 17, 2019 •

edited

Loading