Skip to content
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

Fix objects with functions - replace (objToCss) with (objToCssArray) #2489

Merged
merged 9 commits into from May 29, 2019

Conversation

@d7my11
Copy link
Contributor

@d7my11 d7my11 commented Apr 9, 2019

Fixes: #2468
Addresses: #2414


The issue was in objToCss method that didn't handle methods for objects.

  • Replace objToCss with objToArray which handles methods as well.
  • Cover createGlobalStyle and styled with more test cases.
@d7my11 d7my11 changed the title Fix objects with methods - replace ObjectToCss => ObjectToArray [WIP] Fix objects with methods - replace ObjectToCss => ObjectToArray Apr 9, 2019
@mrmckeb
Copy link

@mrmckeb mrmckeb commented Apr 10, 2019

This is great work, thanks! Out of interest, are there any performance metrics around a) objects/functions over template literals, and b) methods as properties?

@d7my11 d7my11 changed the title [WIP] Fix objects with methods - replace ObjectToCss => ObjectToArray Fix objects with functions - replace (objToCss) with (objToCssArray) Apr 11, 2019
@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented Apr 11, 2019

@mrmckeb I'm not 100% sure actually, but here is my understanding:
using template literal will be converted by babel to arrays if they have embedded expressions and get .concat()ed at the end.

Using objects gets converted by styled-components and it gets slower with nested objects because you have to go through every key for each object.

So, based on my assumptions, using template literal is faster specially if the object contains nested objects.

I would like to hear from somebody who is fully aware of the project to verify or correct me if i'm wrong.

@mrmckeb
Copy link

@mrmckeb mrmckeb commented Apr 12, 2019

Good information, thanks @d7my11 - and great work.

@probablyup
Copy link
Contributor

@probablyup probablyup commented May 1, 2019

I'd also love if you could do this against canary instead or open a second PR for it.

@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented May 2, 2019

@probablyup are there any minor releases for bugs before merging canary into master? if so, will open a second PR for this. If not, will change the base branch.

@probablyup
Copy link
Contributor

@probablyup probablyup commented May 2, 2019

@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented May 2, 2019

Will open another PR then

@probablyup
Copy link
Contributor

@probablyup probablyup commented May 2, 2019

If you could build it on top of @kitten's latest PR that'd be great as I think he made some changes to code near what you're changing

@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented May 13, 2019

@probablyup opened another PR for canary #2542

@probablyup
Copy link
Contributor

@probablyup probablyup commented May 29, 2019

@d7my11 can you get this rebased against master? Looks like a test file just needs a little fix

@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented May 29, 2019

const rules = []
const keys = Object.keys(obj)

keys
Copy link
Contributor

@probablyup probablyup May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be cleaner as a reduce I think

Copy link
Contributor Author

@d7my11 d7my11 May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but it's slower than .forEach because the result has a different type than the items. i did a quick benchmark for that here

Copy link
Contributor

@probablyup probablyup May 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

@probablyup
Copy link
Contributor

@probablyup probablyup commented May 29, 2019

Last thing, a changelog entry would be lovely

@probablyup probablyup added the 4.0 label May 29, 2019
Copy link
Contributor

@probablyup probablyup left a comment

LGTM and thanks!

@probablyup probablyup merged commit f1c8feb into styled-components:master May 29, 2019
1 check passed
@d7my11
Copy link
Contributor Author

@d7my11 d7my11 commented May 29, 2019

@probablyup thanks for reviewing

@d7my11 d7my11 deleted the fix-object-with-methods branch May 29, 2019
@probablyup probablyup mentioned this pull request May 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants