This repository was archived by the owner on May 14, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 61
Add moduleName option #112
Merged
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
b86c44c
Add importName option
mxstbr e7540b3
Amend importName option README section
emilgoldsmith 483ee49
Add test for other library being linted
mxstbr 5a5f332
Merge branch 'master' into import-name
mxstbr 8f51cd4
Update package-lock.json
mxstbr 85367bd
importName -> moduleName
mxstbr d7ca69d
Add support for relative module names
mxstbr 365c9e8
Remove documentation
mxstbr 2e23505
Use path.basename for windows compat
mxstbr 6b3d20c
Test for invalid module name
mxstbr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import emotion from 'emotion' | ||
|
||
// ⚠️ EMPTY BLOCK ⚠️ | ||
const Button = emotion.div` | ||
|
||
` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,71 @@ | ||
const stylelint = require('stylelint') | ||
const path = require('path') | ||
|
||
const processor = path.join(__dirname, '../src/index.js') | ||
const rules = { | ||
'block-no-empty': true, | ||
indentation: 2, | ||
'rule-empty-line-before': [ | ||
'always-multi-line', | ||
{ | ||
except: ['first-nested'], | ||
ignore: ['after-comment'] | ||
} | ||
], | ||
'selector-type-no-unknown': true | ||
} | ||
|
||
describe('options', () => { | ||
let fixture | ||
let data | ||
|
||
// NOTE beforeEach() runs _after_ the beforeAll() hooks of the describe() blocks, so `fixture` | ||
// will have the right path | ||
beforeEach(done => { | ||
stylelint | ||
.lint({ | ||
files: [fixture], | ||
syntax: 'scss', | ||
config: { | ||
// Set importName option to "emotion" | ||
processors: [[processor, { importName: 'emotion' }]], | ||
rules | ||
} | ||
}) | ||
.then(result => { | ||
data = result | ||
done() | ||
}) | ||
.catch(err => { | ||
console.log(err) | ||
data = err | ||
done() | ||
}) | ||
}) | ||
|
||
describe('importName', () => { | ||
beforeAll(() => { | ||
fixture = path.join(__dirname, './fixtures/options/import-name.js') | ||
}) | ||
|
||
it('should have one result', () => { | ||
expect(data.results.length).toEqual(1) | ||
}) | ||
|
||
it('should use the right file', () => { | ||
expect(data.results[0].source).toEqual(fixture) | ||
}) | ||
|
||
it('should have errored', () => { | ||
expect(data.results[0].errored).toEqual(true) | ||
}) | ||
|
||
it('should have one warning (i.e. wrong lines of code)', () => { | ||
expect(data.results[0].warnings.length).toEqual(1) | ||
}) | ||
|
||
it('should have a block-no-empty as the first warning', () => { | ||
expect(data.results[0].warnings[0].rule).toEqual('block-no-empty') | ||
}) | ||
}) | ||
}) |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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?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.
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 doimport styled from 'emotion'
, and we also check the imports so it works if you doimport other from 'styled-components'
, but at the moment if you combine those two things and doimport 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 itlibrary
?), I decided to go forimportName
because a) I wasn't sure ifpackageName
orlibraryName
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 withimportName
since it's unambiguous. (admittedly aliasing is a niche use case, but it'd be annoying to break users who do that)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 remembered why I called it
importName
added my reasoning in the above comment. I can see whyimportName
could be confusing, since "name" doesn't unambigously point to either parts of the import statement.Babel calls it
importNode.source.value
, so maybeimportSourceName
?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.
Maybe I'm thinking this through too much 🤔
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.
Hmm okay, I think I get you now. My main points of misunderstanding was mostly from this line in the README:
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!