rewrite styled import to "no tags" entry #150
Conversation
| */ | ||
| export default path => { | ||
| if (path.node.source.value === 'styled-components') { | ||
| path.node.source = t.stringLiteral('styled-components/no-tags') |
There was a problem hiding this comment.
This means though that we’re currently breaking styled-components/native and /primitives if we’d merge this and this starts being on by default?
Nevermind, reviewing on mobile sucks
There was a problem hiding this comment.
I think as of right now it would only break styled-components/native due to the implicit resolution from https://github.com/styled-components/styled-components/blob/f401eda35ef16065509ea8a7375290fb8b5858a6/package.json#L9
Maybe we should remove that and just have people do this again
import styled from 'styled-components/native'There was a problem hiding this comment.
Seems safer overall and less prone to bundler wonkiness
There was a problem hiding this comment.
@probablyup yea, sorry, the comment was half-baked anyway.
I’m not sure whether we should be going back on that change or not. Currently I’m even thinking tempting people to use universal switches by suggesting it ourselves is bad.
The reality is React Native and Web are extremely different and shared styles should be explicit and mindful. So we should probably deprecate it again. The alternative would be to add a no-tags.native.js to the index to fix it up automatically, which I think is a neat compromise for now.
Long term the RN library should maybe even be separate from styles-components tbh. API-parity is important, sure, but I’d rather have RN specific changes and a playground there and port stuff manually.
cc @mxstbr
There was a problem hiding this comment.
no-tags.native.js
Yeah I can definitely do that. I'm 50/50 on what to do about native in general. Keeping it in the same repo definitely helps keep the API in sync, but it's true that how it works is so very different.
There was a problem hiding this comment.
Yeah honestly we should probably revert that change 😕 It's just made typing impossible and I don't think we should encourage that usage...
|
The whitelist is based on if the underlying component is a string tag or
not, I think it’s fine?
…On Sun, Aug 19, 2018 at 11:16 AM Max Stoiber ***@***.***> wrote:
***@***.**** requested changes on this pull request.
I just realized that the tagless calls don't have the attribute whitelist,
right? I think styled('button') doesn't filter attributes, while
styled.button will—meaning we shouldn't be doing this automatically from
the Babel plugin since it will break things?!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#150 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAiy1kQrK6k-p3y4rh6gs7ggv8rbbJlzks5uSY9vgaJpZM4V5mTN>
.
|
mxstbr
dismissed
their stale review
Oh shit you're right, nevermind: https://codesandbox.io/s/82w3xoj3x9
|
@probablyup @mxstbr yea, I think it just checks whether the target is a string. I don’t think any constructWithOptions path has knowledge of whether it was called from a shorthand. |
Depends on styled-components/styled-components#1912merged