-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
reimplement component selectors #474
Conversation
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?
@@ -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)}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
t.identifier('target'), | ||
t.stringLiteral(stableClassName) | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What bugs around hash generation are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
styled-components/babel-plugin-styled-components#66 Is the big one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The old code appears to use identifierName as well:
getComponentId(state, getName(getIdentifierName(path, t), 'css')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm purely referring to the code above where the identifier name is not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would tracking position not cause the same issue if babel is rewriting the variable assignment and potentially introducing more lines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nvm I think I get it now. It's using an incrementor to just refer vaguely to the nth component in the file, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@probablyup yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/emotion-utils/src/index.js
Outdated
@@ -7,6 +7,13 @@ export function memoize(fn) { | |||
} | |||
} | |||
|
|||
export const KEY_STYLES = '__emotion_styles' | |||
export const KEY_TARGET = '__emotion_target' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename these to STYLES_KEY
and TARGET_KEY
respectively so it's easier to immediately understand what they're for?
packages/emotion-utils/src/index.js
Outdated
|
||
export function isEmotionElement(fn) { | ||
return KEY_STYLES in fn | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you inline this function and use fn[STYLES_KEY] !== undefined
instead of in
?
packages/emotion/src/index.js
Outdated
if (isEmotionElement(interpolation)) { | ||
if ( | ||
process.env.NODE_ENV !== 'production' && | ||
!(KEY_TARGET in interpolation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use !== undefined
as above.
You may want to use some of the tests and stuff that was removed in #334 |
Codecov Report
|
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
@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 |
const stableClassName = getName( | ||
`${hashString( | ||
normalizeFilename(state.file.opts.filename) | ||
)}-${positionInFile}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/react-emotion/src/index.js
Outdated
} | ||
|
||
Styled.withComponent = (nextTag, nextOptions: { target: string } = {}) => { | ||
return createStyled(nextTag, { ...options, ...nextOptions })(styles) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this to
Styled.withComponent = (nextTag, nextOptions?: { target: string }) => {
return createStyled(
nextTag,
nextOptions !== undefined
? omitAssign(testAlwaysTrue, {}, options, nextOptions)
: options
)(styles)
}
packages/react-emotion/src/index.js
Outdated
Styled.withComponent = nextTag => { | ||
return createStyled(nextTag, options)(styles) | ||
if (stableClassName) { | ||
Styled[TARGET_KEY] = stableClassName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to wrap this in an if
.
packages/react-emotion/src/index.js
Outdated
const createStyled = (tag, options: { e: string, label: string }) => { | ||
const createStyled = ( | ||
tag, | ||
options: { e: string, label: string, target: string } = {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use default parameters as the ES5 output will be larger.
color: red; | ||
} | ||
` | ||
}).toThrow() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
packages/emotion-utils/src/index.js
Outdated
export const STYLES_KEY = '__emotion_styles' | ||
export const TARGET_KEY = '__emotion_target' | ||
|
||
export function isEmotionElement(fn) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
t.objectExpression([ | ||
buildTargetObjectProperty(path, state, t) | ||
]) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this is done on the exit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this to the table in the babel-plugin-emotion
README?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Great work @probablyup! Thank you for your contribution.
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>
.
|
Great work @probablyup 👏 Have your figured out |
|
Isnt Component as Selector transformed to a static class name? |
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. |
Ok, gotcha - having runtime info would solve the problem, right? That's of course tougher to achieve with babel. |
What do you mean by "runtime info"? |
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? |
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.
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. |
Fixes #461
Checklist: