Add .extend and .withComponent deterministic ID generation #1044

gregberge · 2017-07-28T10:03:38Z

Will fix #1031

I added a test with the new behaviour. This should fix server-side rendering issues with .extend and .withComponent.

kitten · 2017-07-28T10:25:40Z

src/test/expanded-api.test.js

@@ -62,6 +62,19 @@ describe('expanded api', () => {
      expect(Comp2.styledComponentId).toBe('Comp2-OMGLOL')
      expect(shallow(<Comp2 />).prop('className')).toMatch(/Comp2-OMGLOL/)
    })
+
+    it('should work with `.extend` and `.withComponent`', () => {


Can we separate the .extend from the .withComponent tests? That should make these tests a bit shorter :)

kitten · 2017-07-28T10:26:31Z

src/models/StyledComponent.js

+        const newComponentId =
+          previousComponentId &&
+          `${previousComponentId}-${ComponentStyle.generateName(
+            newRules.join(''),


These rules might contain some interpolations. I'm not sure whether it's a good idea to blindly join them 🤔 cc @geelen

@philpl what do you suggest?

gregberge · 2017-07-28T11:44:16Z

@philpl I modified the .extend, I wasn't taking extended rules. Making a .join('') is not a problem, you can see that it work in test. But I can also skip functions or replace it by a string "FUNC" if you prefer.

gregberge · 2017-07-28T11:49:35Z

@philpl I made changes to support function interpolations

kitten · 2017-07-28T12:44:59Z

@neoziro That is looking fine. The only thing I'm concerned about is performance in this case 😉 I'll take a look later and see whether it's sufficient

kitten · 2017-07-28T12:46:51Z

src/models/StyledComponent.js

          rules: newRules,
          ParentComponent: StyledComponent,
        }

-        return constructWithOptions(createStyledComponent, target, newOptions)
+        const createWithHash = (tag, opts, css) => {


This is not quite looking optimal, in terms of V8 preoptimisation. It will probably deopt this function, since it's created on every call. Any reason why you were unhappy with your previous implementation?

@philpl previously the hash was not built with the rules of .extend. It was built with the rule of the original component. That is not what we want. This is the only way to do it. Also about performances, I think we can do this only if we have a component id.

@neoziro In this case we might want to think about a new option instead. Something like parentId which just passes down the old componentId. Then we let componentId be undefined and inside the StyledComponent factory we pass the parentId into generateId and prepend it to the new id.

cc @geelen

@philpl I don't know the entire codebase in mind, I let you decide what is the best! Should I introduce a parentId concept or I let you do it?

@neoziro Since you're already fully at it and getting your hands dirty, I can guide you to what I mean 😄

Here we could just pass in the componentId as a parentComponentId: https://github.com/styled-components/styled-components/blob/master/src/models/StyledComponent.js#L229

Then it'd eventually arrive back in the factory here: https://github.com/styled-components/styled-components/blob/master/src/models/StyledComponent.js#L178

There we can add a new argument to generateId:

const { componentId = generateId(options.displayName, options.parentComponentId) } = options

Then inside generateId you can concatenate this with displayName to make the new component id "more unique" i.e. different from the parent's one: https://github.com/styled-components/styled-components/blob/master/src/models/StyledComponent.js#L26

One thing we also need to do (probably not in this PR) is passing in the runtime inferred (default) displayName into generateId as well instead of just options.displayName: https://github.com/styled-components/styled-components/blob/master/src/models/StyledComponent.js#L177

@philpl I understood everything, will do it soon

gregberge · 2017-07-29T11:25:01Z

@philpl done

kitten · 2017-07-29T20:43:35Z

src/models/StyledComponent.js

@@ -32,7 +32,8 @@ export default (ComponentStyle: Function, constructWithOptions: Function) => {
    identifiers[displayName] = nr

    const hash = ComponentStyle.generateName(displayName + nr)
-    return `${displayName}-${hash}`
+    const componentId = `${displayName}-${hash}`
+    return parentComponentId ? `${parentComponentId}-${componentId}` : componentId


Maybe parentComponentId !== undefined? Sorry for the nit, but we have an ongoing perf pr and it'd be good to keep things optimised 😅

kitten · 2017-07-29T20:44:33Z

src/models/StyledComponent.js

+
+        const newComponentId =
+          previousComponentId &&
+          `${previousComponentId}-${isTag(tag) ? tag : getComponentName(tag)}`


Maybe we should extract this logic into a small function at the top of this file, now that it's used twice?

This is not exactly the same, so I prefer to keep it separated.

kitten · 2017-07-29T20:45:40Z

@neoziro This is looking perfect! 🎉 I added two small nits. I'll test this and see if it fixes the SSR issues, but I don't see why it wouldn't 😄

gregberge · 2017-07-30T17:30:17Z

@philpl ready to be merged

kitten · 2017-07-30T18:45:26Z

Awesome! 🎉

mxstbr · 2017-07-31T05:32:24Z

Yay! 🎉

Add .extend and .withComponent deterministic ID generation

3b277cf

gregberge mentioned this pull request Jul 28, 2017

Invalid checksum styled-components/babel-plugin-styled-components#66

Closed

kitten requested changes Jul 28, 2017

View reviewed changes

gregberge added 2 commits July 28, 2017 13:39

Fix .extend id generation

bb821f0

Add entry in changelog

688ee5f

Be more deterministic with function

acd3aeb

kitten reviewed Jul 28, 2017

View reviewed changes

Introduce parentComponentId for extend

cfd0787

kitten reviewed Jul 29, 2017

View reviewed changes

kitten approved these changes Jul 29, 2017

View reviewed changes

kitten reviewed Jul 29, 2017

View reviewed changes

gregberge added 2 commits July 30, 2017 17:13

Use strict equality

359f8e0

Merge branch 'master' into deterministic-id-generation

c8a92ca

kitten merged commit ccd293b into styled-components:master Jul 30, 2017

gregberge deleted the deterministic-id-generation branch July 30, 2017 19:05

kitten mentioned this pull request Aug 4, 2017

(client) ss="user-button sc-bdVaJa hJPaNh" data-r (server) ss="user-button sc-bwzfXH eerwGk" data-r #1053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add .extend and .withComponent deterministic ID generation #1044

Add .extend and .withComponent deterministic ID generation #1044

gregberge commented Jul 28, 2017

kitten Jul 28, 2017

kitten Jul 28, 2017

gregberge Jul 28, 2017

gregberge commented Jul 28, 2017

gregberge commented Jul 28, 2017

kitten commented Jul 28, 2017

kitten Jul 28, 2017

gregberge Jul 28, 2017

kitten Jul 28, 2017

gregberge Jul 28, 2017

kitten Jul 28, 2017

gregberge Jul 28, 2017

gregberge commented Jul 29, 2017

kitten Jul 29, 2017

gregberge Jul 30, 2017

kitten Jul 29, 2017

gregberge Jul 30, 2017

kitten commented Jul 29, 2017

gregberge commented Jul 30, 2017

kitten commented Jul 30, 2017

mxstbr commented Jul 31, 2017

Add .extend and .withComponent deterministic ID generation #1044

Add .extend and .withComponent deterministic ID generation #1044

Conversation

gregberge commented Jul 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregberge commented Jul 28, 2017

gregberge commented Jul 28, 2017

kitten commented Jul 28, 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

gregberge commented Jul 29, 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

kitten commented Jul 29, 2017

gregberge commented Jul 30, 2017

kitten commented Jul 30, 2017

mxstbr commented Jul 31, 2017