Add moduleName option #112

mxstbr · 2017-08-28T13:32:16Z

MERGE #111 FIRST

Closes #109

emilgoldsmith · 2017-08-28T15:05:49Z

src/utils/styled.js

-const isStyledImport = node =>
-  node.type === 'ImportDeclaration' && node.source.value === 'styled-components'
+const isStyledImport = (node, importName) =>
+  node.type === 'ImportDeclaration' && node.source.value === importName


Hmm in the README you're saying this depends on the name you import it with, but here we check the library imported, so you should probably update that in the README. Our code is actually agnostic to the variable names you assign them to, as far as I understand anyway. So therefore if you want to use it with another library you would have to specify that option, as we never check for variables called styled I believe.

In that case the argument here and the option name should probably be libraryName, yeah?

Read the associated issue, I just phrased the README badly.

We default to styled for the assigned variable name of the default export, so it'll work if you do import styled from 'emotion', and we also check the imports so it works if you do import other from 'styled-components', but at the moment if you combine those two things and do import other from 'emotion' it'll break. That's what this PR fixes, basically.

Would appreciate PRs improving the README!

Maybe libraryName is the better name for the option (might as well also make it library?), I decided to go for importName because a) I wasn't sure if packageName or libraryName were clearer, and b) if you alias stuff with webpack or something it's not the library name but the name that it's imported from, so I just went with importName since it's unambiguous. (admittedly aliasing is a niche use case, but it'd be annoying to break users who do that)

I remembered why I called it importName added my reasoning in the above comment. I can see why importName could be confusing, since "name" doesn't unambigously point to either parts of the import statement.

Babel calls it importNode.source.value, so maybe importSourceName?

Maybe I'm thinking this through too much 🤔

Hmm okay, I think I get you now. My main points of misunderstanding was mostly from this line in the README:

If you want to avoid edge cases when you assing the default import to a variable not called "styled"

As I think that's super misleading as it actually has nothing to do with what you name your the variable you import it into (it would also work, as I know now you didn't have an understanding problem with, if you did import wabalubadubdub from 'styled-components).

And then I also had a problem because I never heard of the webpack aliasing thing because I never used it, when you talked about aliasing I thought you might've meant import { injectGlobal as some_alias } or stuff like that which again wouldn't have been relevant but I researched the webpack alias thing, and get you now!

Closes #109

coveralls · 2017-08-28T16:00:47Z

Coverage remained the same at 100.0% when pulling b86c44c on import-name into 0bc8560 on master.

emilgoldsmith

Okay I understand you now, you can see my associated comment with this review.

Yeah I think finding the right name for the option is a bit important... I don't think we should be too afraid of the webpack alias edge case as we could either add a small clarifying note in the README or something, but of course if we could have a good name that just made it clear from the get go that would be awesome.

What about... libraryPath?... or... importPath? I feel like adding the path to the name makes it a bit clearer that we're talking about the right side of the from in the import statement, no?
Any other thoughts? I think I quite like libraryPath actually, I think it's quite descriptive, though it could of course be misunderstood as the real path including ./node_modules etc. though that would of course work if someone decides to actually import it from there, so maybe importPath? Yeah, it's not a straight forward naming problem.

Though we of course now will actually have to do the final README PR on the website repo I'll just add a commit here with what I feel like are some relevant clarifications to the README and when we decide we can delete the README ammends from this PR and open a PR on the website repo

emilgoldsmith · 2017-09-02T14:58:07Z

src/utils/styled.js

-const isStyledImport = node =>
-  node.type === 'ImportDeclaration' && node.source.value === 'styled-components'
+const isStyledImport = (node, importName) =>
+  node.type === 'ImportDeclaration' && node.source.value === importName


Hmm okay, I think I get you now. My main points of misunderstanding was mostly from this line in the README:

If you want to avoid edge cases when you assing the default import to a variable not called "styled"

As I think that's super misleading as it actually has nothing to do with what you name your the variable you import it into (it would also work, as I know now you didn't have an understanding problem with, if you did import wabalubadubdub from 'styled-components).

And then I also had a problem because I never heard of the webpack aliasing thing because I never used it, when you talked about aliasing I thought you might've meant import { injectGlobal as some_alias } or stuff like that which again wouldn't have been relevant but I researched the webpack alias thing, and get you now!

emilgoldsmith · 2017-09-02T15:10:54Z

Okay, README updated with a suggestion for change :)

coveralls · 2017-09-02T15:20:39Z

Coverage remained the same at 100.0% when pulling e7540b3 on import-name into 0bc8560 on master.

mxstbr · 2017-09-02T16:19:35Z

I like the change, it's not entirely accurate though, because all these cases work at the moment:

import styled from 'styled-components';

// ✅ This is linted! 
const Button = styled.div`
  color: #PP0;
`

// ----

import somethingElse from 'styled-components';

// ✅ This is linted! 
const Button = somethingElse.div`
  color: #PP0;
`

// ----

import styled from 'some-other-lib';

// ✅ This is also linted! 
const Button = styled.div`
  color: #PP0;
`

It's just that the case breaks where you change the import name of a different library:

import somethingElse from 'some-other-lib';

// ❌ This isn't linted
const Button = somethingElse.div`
  color: #PP0;
`

So that's what I'm trying to fix here.

Re: naming, I agree with your point, I think "path" makes it a bit confusing though. What do you think about "moduleName"?

emilgoldsmith · 2017-09-02T19:12:33Z

Interesting, because my whole "disagreement" with some of the things you wrote here is based on me being pretty sure that we don't lint the last case you gave, this one:

import styled from 'some-other-lib';

// ✅ This is also linted! 
const Button = styled.div`
  color: #PP0;
`

Are you really sure we do that? Can you point me to the line of code that picks up on this? Because isStyledImport just checks if the imported name is styled-components so that's what I've based my thoughts on in this discussion.

mxstbr · 2017-09-02T19:29:40Z

I'm fairly certain the isStyledImport thing only checks it to change the variable name that's imported but doesn't break if it doesn't find the library? Maybe that's incorrect though?

emilgoldsmith · 2017-09-02T19:33:24Z

I'm like 95% sure you're incorrect. That's also why I've been correcting you this whole time, try and test it, I'm really sure it doesn't care about absolutely anything but the importPath or w/e we call it, as we use the importNames or something like that dictionary that keeps a map of what the variable names actually are.

I could be wrong but I'd be super surprised from my current understanding of our codebase

mxstbr · 2017-09-02T19:35:40Z

Okay, I'm adding a test right now!

mxstbr · 2017-09-02T19:37:25Z

I've added a test in 483ee49 and it passes. (at least locally) Did I do something wrong with it?

coveralls · 2017-09-02T19:44:12Z

Coverage remained the same at 100.0% when pulling 483ee49 on import-name into 0bc8560 on master.

mxstbr · 2017-09-02T19:44:53Z

I'm pretty certain that's the way it works, honestly. I'd be surprised to find out it doesn't work that way. The only thing the isStyledImport thing does is check for the import other from 'styled-components' case, not for the actual linting.

emilgoldsmith · 2017-09-02T20:14:17Z

Okay I get it now, you are right, but yeah, it's kinda subtle. Since we have default values for the importedNames object and we never overwrite anything due to isStyledImport(node) never evaluating to true the defaults are kept, and the default is, as you say, styled so I now understand that it is indeed how it works and why.

On a different note though (it's a different discussion whether this is high priority) I don't think I like that implementation though, so right now we don't care about anything but the variable name? So if anyone calls any TTL with the names styled, css, keyframes, or injectGlobal we will be linting whatever is inside? I find that very bad behaviour. In this case even

const styled = (text) => console.log(text)
styled`hello, what's up`

would be linted and throw a lot of errors. IMO we should not have defaults and only instantiate importedNames if isStyledImport returns true and gives us the actual imported variable names, and then also implement the option we talk about in this PR, and then in that case my README would also be correct as this is how I thought it worked before.
But that's just my 2 cents of course.

mxstbr · 2017-09-02T21:02:52Z

I decided to go down this route because I can't prescribe what/if people use module bundlers. Some folks might use global variables, and that's perfectly fine!

The reason I kinda doubt that this is an issue is 1) because it's never come up and 2) if you put code like this in your codebase that's linted with the styled components processor maybe you deserve an error:

styled.div`
  not css here this is just a string
`

css`
  nah mate, css is not here
`

injectGlobal`
  yeah no this is definitely not CSS
`

The only one I'm 50/50 about is injectGlobal where I could maybe see people using that name for something else but it's so highly unlikely that I think it's worth not being finnicky about build systems and module bundlers. Note that for the error to occur you have to manually add the stylelint setup AND you have to manually copy the styled-components API but change the way it works. I just find that highly unlikely/user fault, I'd rather support styled-components alternatives.

Happy to talk about it if you disagree though, let's open an issue?

To get back on track, what name do we go with now? 😆

emilgoldsmith · 2017-09-02T23:55:24Z

Yeah okay, I guess I can see some logic in this. Maybe I'll add an explanatory comment to the code the next time I'm touching parsers/index.js as I think the intention behind it could be a bit clearer in the code :). Thanks for taking the time for this long discussion haha.

For the name.... yes... moduleName might be quite good actually. I dunno, it doesn't quite take into account the path stuff, but yeah, you know we could even do a small implementation thing where we also check if the last part of the path equals the module name so one didn't need to do something with a path if you for some reason used a path. Like a check like ORIGINAL CHECK || source.split('/')[source.split('/').length-1] === moduleName kinda thing (and do another || and check with \ as well). That would both be nicer behavior in terms of user experience I feel, and also it could fit well with naming it moduleName as it could then be path ambiguous.
What do you say to that?

And for another one! I think I'll just write about this in Spectrum, but I feel like we're pretty much ready to publish 1.0 now right? We can continue this in Spectrum

mxstbr · 2017-09-05T09:17:48Z

I forgot about relative paths, but I like that. I'll go ahead and implement this quick!

coveralls · 2017-09-05T09:25:25Z

Coverage remained the same at 100.0% when pulling f41899f on import-name into 6984963 on master.

coveralls · 2017-09-05T09:31:21Z

Coverage remained the same at 100.0% when pulling 85367bd on import-name into 6984963 on master.

coveralls · 2017-09-05T09:33:42Z

Coverage remained the same at 100.0% when pulling 85367bd on import-name into 6984963 on master.

Updated code

mxstbr · 2017-09-05T09:40:24Z

This should be ready to go once tests pass! Supports both these cases:

// ----absolute----
import emotion from 'emotion'
const Bla = emotion.div``

// ----relative----
import emotion from '../../emotion'
const Bla = emotion.div``

mxstbr · 2017-09-05T09:41:06Z

Oh wait, gotta update the docs.

coveralls · 2017-09-05T09:46:29Z

Coverage remained the same at 100.0% when pulling 365c9e8 on import-name into 6984963 on master.

@emilgoldsmith

Adds documentation about the new `moduleName` option of the stylelint processor. **DO NOT MERGE UNTIL styled-components/stylelint-processor-styled-components#112 IS MERGED!** /cc @emilgoldsmith @ismay

mxstbr · 2017-09-05T09:47:51Z

Submitted PR to add the documentation for this option to the website here: styled-components/styled-components-website#107

This PR is good to go!

coveralls · 2017-09-05T09:53:21Z

Coverage remained the same at 100.0% when pulling 365c9e8 on import-name into 6984963 on master.

emilgoldsmith

Basically good! I like this as well, just as in the above comment maybe make sure to support Windows paths?

And then maybe also add a test for a library that doesn't get picked up, such as import not-styled from 'some-other-library' just so we have that case checked as well.

emilgoldsmith · 2017-09-05T12:13:12Z

src/utils/styled.js

-  node.type === 'ImportDeclaration' && node.source.value === 'styled-components'
+const isStyledImport = (node, moduleName) =>
+  node.type === 'ImportDeclaration' &&
+  node.source.value.split('/')[node.source.value.split('/').length - 1] === moduleName


Yeah this is good :).

The only thing is maybe we should also do an || with \ for Windows? Or maybe the path module can do something like this for us where it handles all the edge cases?

Good points; used path.basename for windows compat and added a test for an invalid module name!

coveralls · 2017-09-05T13:07:03Z

Coverage remained the same at 100.0% when pulling 6b3d20c on import-name into 6984963 on master.

coveralls · 2017-09-05T13:16:34Z

Coverage remained the same at 100.0% when pulling 6b3d20c on import-name into 6984963 on master.

emilgoldsmith

Perfect :). I'm all good with this and the documentation being merged in now

mxstbr · 2017-09-05T14:25:43Z

Awesome!

emilgoldsmith previously requested changes Aug 28, 2017

View reviewed changes

mxstbr changed the base branch from 1.0 to master August 28, 2017 15:52

Add importName option

b86c44c

Closes #109

mxstbr force-pushed the import-name branch from 4e40a29 to b86c44c Compare August 28, 2017 15:53

emilgoldsmith reviewed Sep 2, 2017

View reviewed changes

Amend importName option README section

e7540b3

Add test for other library being linted

483ee49

mxstbr added 2 commits September 5, 2017 11:18

Merge branch 'master' into import-name

5a5f332

Update package-lock.json

8f51cd4

importName -> moduleName

85367bd

mxstbr force-pushed the import-name branch from f41899f to 85367bd Compare September 5, 2017 09:25

Add support for relative module names

d7ca69d

mxstbr changed the title ~~Add importName option~~ Add moduleName option Sep 5, 2017

mxstbr requested a review from emilgoldsmith September 5, 2017 09:39

Remove documentation

365c9e8

mxstbr mentioned this pull request Sep 5, 2017

Add documentation about moduleName option styled-components/styled-components-website#107

Merged

emilgoldsmith previously requested changes Sep 5, 2017

View reviewed changes

mxstbr added 2 commits September 5, 2017 14:56

Use path.basename for windows compat

2e23505

Test for invalid module name

6b3d20c

mxstbr requested a review from emilgoldsmith September 5, 2017 13:05

emilgoldsmith approved these changes Sep 5, 2017

View reviewed changes

mxstbr merged commit aac20ac into master Sep 5, 2017

mxstbr deleted the import-name branch September 5, 2017 14:27

mxstbr mentioned this pull request Jan 22, 2018

moduleName option styled-components/babel-plugin-styled-components#120

Open

Add moduleName option #112

Add moduleName option #112

Conversation

mxstbr commented Aug 28, 2017 • edited Loading

emilgoldsmith Aug 28, 2017

Choose a reason for hiding this comment

mxstbr Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

mxstbr Aug 28, 2017

Choose a reason for hiding this comment

mxstbr Aug 28, 2017

Choose a reason for hiding this comment

emilgoldsmith Sep 2, 2017

Choose a reason for hiding this comment

coveralls commented Aug 28, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

emilgoldsmith Sep 2, 2017

Choose a reason for hiding this comment

emilgoldsmith commented Sep 2, 2017

coveralls commented Sep 2, 2017

mxstbr commented Sep 2, 2017

emilgoldsmith commented Sep 2, 2017

mxstbr commented Sep 2, 2017

emilgoldsmith commented Sep 2, 2017

mxstbr commented Sep 2, 2017

mxstbr commented Sep 2, 2017

coveralls commented Sep 2, 2017

mxstbr commented Sep 2, 2017

emilgoldsmith commented Sep 2, 2017

mxstbr commented Sep 2, 2017 • edited Loading

emilgoldsmith commented Sep 2, 2017

mxstbr commented Sep 5, 2017

coveralls commented Sep 5, 2017

coveralls commented Sep 5, 2017

coveralls commented Sep 5, 2017

mxstbr commented Sep 5, 2017 • edited Loading

mxstbr commented Sep 5, 2017

coveralls commented Sep 5, 2017

mxstbr commented Sep 5, 2017 • edited Loading

coveralls commented Sep 5, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

emilgoldsmith Sep 5, 2017

Choose a reason for hiding this comment

coveralls commented Sep 5, 2017

coveralls commented Sep 5, 2017

emilgoldsmith left a comment

Choose a reason for hiding this comment

mxstbr commented Sep 5, 2017

mxstbr commented Aug 28, 2017 •

edited

Loading

mxstbr Aug 28, 2017 •

edited

Loading

mxstbr commented Sep 2, 2017 •

edited

Loading

mxstbr commented Sep 5, 2017 •

edited

Loading

mxstbr commented Sep 5, 2017 •

edited

Loading