Fix early return for cached false values in importLocalName

[ZauberNerd]

Closes: #232

[ZauberNerd]

Before this change the value in the cache would be false and therefore this if condition would never evaluate truthy and we would traverse all imports for files that are not part of styled-components every time again.

[quantizor]

Hmm I'm confused when this would ever happen... is it possible for the same babel plugin to be run in parallel on the same file? The only place where the cached value could be false is in the css prop transpile visitor, and it immediately injects and re-caches the injected import...

[ZauberNerd]

@probablyup as per your comment (#230 (comment)) it is false for all ESM files:

babel-plugin-styled-components/src/utils/detectors.js

Lines 20 to 24 in 2bfc83a

	let localName = state.styledRequired
	? name === 'default'
	? 'styled'
	: name
	: false

because as far as I can tell styledRequired will only be set for commonJS files (https://github.com/styled-components/babel-plugin-styled-components/blob/e04eb6f264f89a8abe5f32b0ca22dc200fe7e360/src/visitors/assignStyledRequired.js).
And then localName is initially set to false (

babel-plugin-styled-components/src/utils/detectors.js

Line 24 in 2bfc83a

: false

) and if the file has no styled-components import (

babel-plugin-styled-components/src/utils/detectors.js

Line 31 in 2bfc83a

if (isValidTopLevelImport(node.source.value)) {

) it will never re-define the localName variable, therefore the cache value of localNameCache[cacheKey] will be false (

babel-plugin-styled-components/src/utils/detectors.js

Line 53 in 2bfc83a

localNameCache[cacheKey] = localName

) which means that this if condition won't evaluate truthy (https://github.com/styled-components/babel-plugin-styled-components/blob/v1.10.5/src/utils/detectors.js#L16).

[quantizor]

All that is true but the only time localName will ever stay false is in the use case of the css prop which doesn't require styled to be in scope (except the babel macro use case.) Otherwise localName has to be set to something picked up via ImportDeclaration traversal because you can't use the normal features of styled-components without importing it.

There is a code smell here...

[quantizor]

If you use yarn, could you do me the favor of reverting to 1.10.4 in your project and adding a "resolutions" field to your master package.json to force any installed versions of babel-plugin-styled-components to the same version? e.g.

"resolutions": {
  "babel-plugin-styled-components": "1.10.4"
}

[ZauberNerd]

@probablyup I think you are right, but the problem is all other files that do not use styed-components (in which case the localName would stay false). For me it looks like this traversal (

babel-plugin-styled-components/src/utils/detectors.js

Lines 26 to 51 in 2bfc83a

	state.file.path.traverse({
	ImportDeclaration: {
	exit(path) {
	const { node } = path
	if (isValidTopLevelImport(node.source.value)) {
	for (const specifier of path.get('specifiers')) {
	if (specifier.isImportDefaultSpecifier()) {
	localName = specifier.node.local.name
	}
	if (
	specifier.isImportSpecifier() &&
	specifier.node.imported.name === name
	) {
	localName = specifier.node.local.name
	}
	if (specifier.isImportNamespaceSpecifier()) {
	localName = specifier.node.local.name
	}
	}
	}
	},
	},
	})

) is very costly (that's probably why the cache was introduced in the first place).

[ZauberNerd]

@probablyup for my use-case you could try and yarn create hops-app my-app and then edit your node_modules folder with some console.log's or the changes of this PR to see the effect. I think it might even work without these changes but it will take a huge amount of time longer and that is what is breaking the builds (because travis, for example, terminates builds that don't print outpout for longer than 10 minutes).

[quantizor]

Published a test build under babel-plugin-styled-components@test if you want to share it and see if it works for people.

[c0b41]

@probablyup working great for gatsby probably work next js too..

@ZauberNerd great work btw 👍👍👍

[quantizor]

@c0b41 to be clear nextjs has always been working for me. I have 5 nextjs projects using s-c and they never had this issue, so I have a strong suspicion there's a mixed babel environment problem happening that manifests more strongly with certain project setups.

[quantizor]

Alright I think I get it and makes more sense since before the set of changes in 1.10.3 the localName defaulted to a string that evaluated as truthy and the new default is falsy for an ESM setup.