reimplement component selectors #474

quantizor · 2017-11-24T06:39:00Z

Fixes #461

Checklist:

Documentation
Tests
Code complete

Initial pass at the functionality... 1) Should this be supported in extractStatic? 2) How do I convert the interpolation in that case to a static rule, since the target can be easily grabbed?

quantizor · 2017-11-24T06:39:54Z

packages/babel-plugin-emotion/src/index.js

@@ -97,6 +97,17 @@ export function replaceCssWithCallExpression(
 export function buildStyledCallExpression(identifier, tag, path, state, t) {
  const identifierName = getIdentifierName(path, t)

+  let stableClassName = `el-${hashString(state.file.opts.filename)}`


I think filename + identifier name should be unique enough to generate a stable hash per emotion element instance.

The CI file structure is different than local dev, so keeping the path fragments leads to differing hashes.

emmatown · 2017-11-24T09:05:43Z

packages/babel-plugin-emotion/src/index.js

+    t.identifier('target'),
+    t.stringLiteral(stableClassName)
+  )
+


Could you use this code to generate the class name? (especially with the css- prefix so that the hash isn't shown in tests with jest-glamor-react)

emotion/packages/babel-plugin-emotion/src/index.js

Lines 81 to 134 in 54f054a

// babel-plugin-styled-components

// https://github.com/styled-components/babel-plugin-styled-components/blob/37a13e9c21c52148ce6e403100df54c0b1561a88/src/visitors/displayNameAndId.js#L49-L93

const findModuleRoot = filename => {

if (!filename || filename === 'unknown') {

return null

}

let dir = dirname(filename)

if (fs.existsSync(pathJoin(dir, 'package.json'))) {

return dir

} else if (dir !== filename) {

return findModuleRoot(dir)

} else {

return null

}

}

const FILE_HASH = 'emotion-file-hash'

const COMPONENT_POSITION = 'emotion-component-position'

const getFileHash = state => {

const { file } = state

// hash calculation is costly due to fs operations, so we'll cache it per file.

if (file.get(FILE_HASH)) {

return file.get(FILE_HASH)

}

const filename = file.opts.filename

// find module root directory

const moduleRoot = findModuleRoot(filename)

const filePath =

moduleRoot && relative(moduleRoot, filename).replace(pathSep, '/')

let moduleName = ''

if (moduleRoot) {

const packageJsonContent = fs.readFileSync(

pathJoin(moduleRoot, 'package.json')

)

if (packageJsonContent) {

try {

moduleName = JSON.parse(packageJsonContent.toString()).name

} catch (e) {}

}

}

const code = file.code

const fileHash = hashArray([moduleName, filePath, code])

file.set(FILE_HASH, fileHash)

return fileHash

}

const getNextId = state => {

const id = state.file.get(COMPONENT_POSITION) || 0

state.file.set(COMPONENT_POSITION, id + 1)

return id

}

Happy to change el- to css-. Why the more complicated hash function though? styled-components has a lot of bugs around hash generation... I'd rather go with as simple a solution as possible to avoid them and noticed styled uses a lot similar code to what was removed.

The more complicated hash function is required so that it's unique, deterministic and won't change even if the project has a different full path (hence using the path to the nearest package.json).

What bugs around hash generation are you referring to?

styled-components/babel-plugin-styled-components#66 Is the big one

The old code appears to use identifierName as well:

emotion/packages/babel-plugin-emotion/src/index.js

Line 162 in 54f054a

getComponentId(state, getName(getIdentifierName(path, t), 'css'))

I'm purely referring to the code above where the identifier name is not used.

Would tracking position not cause the same issue if babel is rewriting the variable assignment and potentially introducing more lines?

Nvm I think I get it now. It's using an incrementor to just refer vaguely to the nth component in the file, right?

@probablyup yes.

emmatown

I'm happy with this. I was initially skeptical because I assumed that we would make it work without babel-plugin-emotion but I definitely think making it only work with babel-plugin-emotion is the right way to go so that we have deterministic SSR. I think the only thing left that this needs to handle is withComponent and the fact that we can't handle withComponent in the babel macro. The other thing with withComponent is that there is no way to know if we're definitely seeing an emotion withComponent call or something else that has a withComponent property though I'm personally okay with treating all withComponent property calls as if they're from emotion.

emmatown · 2017-11-24T09:21:41Z

packages/emotion-utils/src/index.js

@@ -7,6 +7,13 @@ export function memoize(fn) {
  }
 }

+export const KEY_STYLES = '__emotion_styles'
+export const KEY_TARGET = '__emotion_target'


Could you rename these to STYLES_KEY and TARGET_KEY respectively so it's easier to immediately understand what they're for?

emmatown · 2017-11-24T09:22:45Z

packages/emotion-utils/src/index.js

+
+export function isEmotionElement(fn) {
+  return KEY_STYLES in fn
+}


Could you inline this function and use fn[STYLES_KEY] !== undefined instead of in?

emmatown · 2017-11-24T09:23:27Z

packages/emotion/src/index.js

+      if (isEmotionElement(interpolation)) {
+        if (
+          process.env.NODE_ENV !== 'production' &&
+          !(KEY_TARGET in interpolation)


Use !== undefined as above.

emmatown · 2017-11-24T09:41:52Z

You may want to use some of the tests and stuff that was removed in #334

codecov · 2017-11-25T03:23:52Z

Codecov Report

Merging #474 into master will decrease coverage by 0.05%.
The diff coverage is 96.42%.

Impacted Files	Coverage Δ
packages/emotion-utils/src/index.js	`100% <100%> (ø)`	⬆️
packages/react-emotion/src/index.js	`100% <100%> (ø)`	⬆️
packages/emotion/src/index.js	`100% <100%> (ø)`	⬆️
packages/babel-plugin-emotion/src/index.js	`96.29% <95%> (-0.16%)`	⬇️

one of the subdependencies being used to make the site is shipping their module in ES6 syntax (using `let`) so uglify2 was choking on it. upgraded to the new uglifyjs-webpack-plugin which can handle es6+ syntax and replicated what `webpack -p` does since webpack itself hasn't merged the upgrade yet

quantizor · 2017-11-26T00:40:37Z

@mitchellhamilton any further comments? I think I addressed what you asked for. The only thing I can't figure out is how to make this work with extractStatic, if that matters.

emmatown · 2017-11-26T00:51:43Z

packages/babel-plugin-emotion/src/index.js

+  const stableClassName = getName(
+    `${hashString(
+      normalizeFilename(state.file.opts.filename)
+    )}-${positionInFile}`,


Could you add the code (state.file.code) and the name of the module from the nearest package.json to the hash and remove the dash between the hash and position?

I could, but it would mean that changing the contents of a test file would cause the hashes to change and blow the test snapshots.

For tests that don't use jest-glamor-react, that's correct. We need to use the code so that we have a fallback in case we're in a browser or something else where we don't have a filesystem/a filename and in general, it's another way to ensure that the hash is unique. It'll only affect the internal emotion tests so I don't really think it's a problem to break the snapshots.

emmatown · 2017-11-26T01:02:25Z

packages/react-emotion/src/index.js

+    }
+
+    Styled.withComponent = (nextTag, nextOptions: { target: string } = {}) => {
+      return createStyled(nextTag, { ...options, ...nextOptions })(styles)
    }


Could you change this to

Styled.withComponent = (nextTag, nextOptions?: { target: string }) => { return createStyled( nextTag, nextOptions !== undefined ? omitAssign(testAlwaysTrue, {}, options, nextOptions) : options )(styles) }

emmatown · 2017-11-26T01:04:01Z

packages/react-emotion/src/index.js

-    Styled.withComponent = nextTag => {
-      return createStyled(nextTag, options)(styles)
+    if (stableClassName) {
+      Styled[TARGET_KEY] = stableClassName


There's no need to wrap this in an if.

emmatown · 2017-11-26T01:06:01Z

packages/react-emotion/src/index.js

-const createStyled = (tag, options: { e: string, label: string }) => {
+const createStyled = (
+  tag,
+  options: { e: string, label: string, target: string } = {}


Please don't use default parameters as the ES5 output will be larger.

emmatown · 2017-11-26T01:07:26Z

packages/emotion/test/component-selector/component-selector.test.js

+          color: red;
+        }
+      `
+    }).toThrow()


Could you change this to .toThrowErrorMatchingSnapshot()

Also, these tests don't need to be in a separate folder, we only have tests in a separate folder if they need a custom babel config.

emmatown · 2017-11-26T01:12:52Z

packages/emotion-utils/src/index.js

+export const STYLES_KEY = '__emotion_styles'
+export const TARGET_KEY = '__emotion_target'
+
+export function isEmotionElement(fn) {


Could you please inline this function. I'd rather not expose it, especially in a place where people can import it since it means people could start relying on it. I'm okay with exporting the keys since I think it's clear that they're implementation details.

emmatown · 2017-11-26T01:13:58Z

packages/babel-plugin-emotion/src/index.js

+                  t.objectExpression([
+                    buildTargetObjectProperty(path, state, t)
+                  ])
+                )


Is there a reason this is done on the exit?

It seemed like a safe thing to do, since it's tagging on some additional stuff once the other processing is complete. I can move it to enter though if you'd prefer.

it's tagging on some additional stuff once the other processing is complete

I'm not sure what you mean by this though it shouldn't affect anything in this case so you can just leave it there.

emmatown · 2017-11-26T01:22:48Z

docs/babel.md

@@ -34,3 +34,6 @@ import { css, keyframes, fontFace, injectGlobal, flush, hydrate } from 'emotion/
 ```

 [create-react-app issue discussing macros](https://github.com/facebookincubator/create-react-app/issues/2730).
+
+### Components as selectors
+The ability to refer to another component to apply override styles depending on nesting context. Learn more in the [react-emotion docs](./styled.md#targeting-another-emotion-component).


Could you add this to the table in the babel-plugin-emotion README?

tkh44

LGTM

Great work @probablyup! Thank you for your contribution.

quantizor · 2017-11-26T04:00:26Z

Thanks Mitchell for all the reviews!

…

On Sat, Nov 25, 2017 at 10:47 PM Kye Hohenberger ***@***.***> wrote: Merged #474 <#474>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#474 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAiy1oLOTDMiP4qC_vWoqyQJk7_Oti2Jks5s6N9rgaJpZM4QpbIp> .

Andarist · 2017-11-28T20:04:33Z

Great work @probablyup 👏 Have your figured out extractStatic support? Cannot find a note in the docs that it might not work with this feature, but you have mentioned having problems with this.

tkh44 · 2017-11-28T20:11:44Z

extractStatic could not work because by definition you would have expressions in your style blocks.

Andarist · 2017-11-28T20:15:00Z

Isnt Component as Selector transformed to a static class name?

emmatown · 2017-11-28T22:07:21Z

The class used for targeting components is added to the styled call. To make a component that targets another component be extracted we would have to know an interpolation is a styled component and know where it's defined which would be very hard.

You can use component selectors to target a styled component that is extracted but the one that targets the other will not be extracted.

Andarist · 2017-11-28T22:33:34Z

Ok, gotcha - having runtime info would solve the problem, right? That's of course tougher to achieve with babel.

emmatown · 2017-11-28T22:34:48Z

What do you mean by "runtime info"?

Andarist · 2017-11-28T22:38:14Z

By statically analyzing a single file we don't know what imported things are, right? If we could trace the imports and require them to inspect their type etc it should get easy to use that 'runtime' info to extract more css when possible.

Btw - does it mean that interpolated 'static partials' cannot be extracted at the moment too?

emmatown · 2017-11-28T22:43:21Z

By statically analyzing a single file we don't know what imported things are, right? If we could trace the imports and require them to inspect their type etc it should get easy to use that 'runtime' info to extract more css when possible.

That's correct but tracing imports would get very annoying very quickly since we would have to deal with aliasing and other things like that and in general it would be complex.

Btw - does it mean that interpolated 'static partials' cannot be extracted at the moment too?

Yes, the only things that are extracted right now are things with no interpolations whatsoever.

To be perfectly honest, we don't care about extraction very much since it dramatically limits the power of css-in-js.

reimplement component selectors

ac3ba6c

Initial pass at the functionality... 1) Should this be supported in extractStatic? 2) How do I convert the interpolation in that case to a static rule, since the target can be easily grabbed?

quantizor commented Nov 24, 2017

View reviewed changes

use only the file basename for testing

a17cebb

The CI file structure is different than local dev, so keeping the path fragments leads to differing hashes.

emmatown reviewed Nov 24, 2017

View reviewed changes

emmatown added the Tag: Enhancement label Nov 24, 2017

quantizor added 6 commits November 24, 2017 13:16

fix code style comments

d6948e9

withComponent support, "el-" -> "css-"

c49c672

adjust the filename resolver to be more stable x-arch, add tests

6802b53

Merge remote-tracking branch 'upstream/master' into es-component-sel

d74ed1a

run prettier

1d8c108

bump up bundlesize a tad more

2604fea

quantizor added 4 commits November 24, 2017 22:32

use component position for stable naming instead of the identifier

0631042

fix some errant snapshots

b482ba4

coverage, add the new stable class to component output

97fe0e3

docs

9383c88

quantizor changed the title ~~[WIP] reimplement component selectors~~ reimplement component selectors Nov 25, 2017

quantizor added 3 commits November 24, 2017 23:19

update some snapshots

49367f9

coverage

bc855c1

emmatown reviewed Nov 26, 2017

View reviewed changes

quantizor added 6 commits November 25, 2017 20:27

add component selector to the babel plugin doc table

9c4d367

inline isEmotionElement func

cb402f4

snapshot the component selector error test output

d2dbc26

remove unnecessary guard

86af032

refactor withComponent per PR comment

bf1e432

move component-selector tests up a level

8fb89cb

quantizor added 2 commits November 25, 2017 20:41

remove default params, reduce bundlesize back down

187b300

adjust hash generation per PR comments

ea3e2c8

emmatown approved these changes Nov 26, 2017

View reviewed changes

emmatown requested a review from tkh44 November 26, 2017 02:16

emmatown added the merge me! label Nov 26, 2017

tkh44 approved these changes Nov 26, 2017

View reviewed changes

tkh44 merged commit 6a8311d into emotion-js:master Nov 26, 2017

emmatown mentioned this pull request Nov 26, 2017

Guidance on a replacement for component selectors #446

Closed

quantizor deleted the es-component-sel branch June 25, 2019 02:04

	// babel-plugin-styled-components
	// https://github.com/styled-components/babel-plugin-styled-components/blob/37a13e9c21c52148ce6e403100df54c0b1561a88/src/visitors/displayNameAndId.js#L49-L93

	const findModuleRoot = filename => {
	if (!filename \|\| filename === 'unknown') {
	return null
	}
	let dir = dirname(filename)
	if (fs.existsSync(pathJoin(dir, 'package.json'))) {
	return dir
	} else if (dir !== filename) {
	return findModuleRoot(dir)
	} else {
	return null
	}
	}

	const FILE_HASH = 'emotion-file-hash'
	const COMPONENT_POSITION = 'emotion-component-position'

	const getFileHash = state => {
	const { file } = state
	// hash calculation is costly due to fs operations, so we'll cache it per file.
	if (file.get(FILE_HASH)) {
	return file.get(FILE_HASH)
	}
	const filename = file.opts.filename
	// find module root directory
	const moduleRoot = findModuleRoot(filename)
	const filePath =
	moduleRoot && relative(moduleRoot, filename).replace(pathSep, '/')
	let moduleName = ''
	if (moduleRoot) {
	const packageJsonContent = fs.readFileSync(
	pathJoin(moduleRoot, 'package.json')
	)
	if (packageJsonContent) {
	try {
	moduleName = JSON.parse(packageJsonContent.toString()).name
	} catch (e) {}
	}
	}
	const code = file.code

	const fileHash = hashArray([moduleName, filePath, code])
	file.set(FILE_HASH, fileHash)
	return fileHash
	}

	const getNextId = state => {
	const id = state.file.get(COMPONENT_POSITION) \|\| 0
	state.file.set(COMPONENT_POSITION, id + 1)
	return id
	}

reimplement component selectors #474

reimplement component selectors #474

Conversation

quantizor commented Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

emmatown Nov 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emmatown commented Nov 24, 2017

codecov bot commented Nov 25, 2017 • edited Loading

Codecov Report

quantizor commented Nov 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkh44 left a comment

Choose a reason for hiding this comment

quantizor commented Nov 26, 2017 via email

Andarist commented Nov 28, 2017

tkh44 commented Nov 28, 2017

Andarist commented Nov 28, 2017

emmatown commented Nov 28, 2017 • edited Loading

Andarist commented Nov 28, 2017 • edited Loading

emmatown commented Nov 28, 2017

Andarist commented Nov 28, 2017

emmatown commented Nov 28, 2017

quantizor commented Nov 24, 2017 •

edited

Loading

emmatown Nov 24, 2017 •

edited

Loading

codecov bot commented Nov 25, 2017 •

edited

Loading

emmatown commented Nov 28, 2017 •

edited

Loading

Andarist commented Nov 28, 2017 •

edited

Loading