Move to shared config #49
Conversation
|
Wait, what's missing here? Is this done? We should release this with 1.0! |
|
@mxstbr Definitely. I was waiting for the more significant refactors, but yeah this should definitely be a part of v1 as it'll be a breaking change. I'll update the pr to allow for feedback. But it should be quite simple. |
ismay
force-pushed
the
move-to-shared-config
branch
from
c493139
to
cf988ad
Compare
|
Rebased it on master. The package-lock update is actually unrelated to this pr, but something that always happens for me with greenkeeper lockfile updates. For some reason a regular Anyway. I've added stylelint-config-styled-components-processor as a peerDependency. One thing I've been thinking about is where the config should be tested. Way I see it, is that we could just add some simple tests to the stylelint-config-styled-components-processor repo, just to verify that it's ignoring and enforcing the correct rules. Other than that the todo's above are all I think should be changed. Let me know if there's anything that I've overlooked! |
It could be because the |
|
Updated typescript to the version that typescript-eslint-parser currently supports (2.4.0). I could take it out since it isn't specifically related to this pr, but I was getting a bunch of warnings during testing. Still feels weird to bundle typescript as a dependency btw, when it seems more appropriate as a peerDependency. Maybe good to discuss that in another issue, don't know how you feel about that? But back to the matters at hand: index.js has been updated to no longer ignore any stylelint rules. @emilgoldsmith notice anything I might have missed? Basically I think this is it, and we just need to update the readme and release stylelint-config-styled-components-processor on npm. |
|
Great thing is that now we're also not relying too much on the internal structure of the stylelintResult anymore. Only thing we're doing is replace brace with backtick, which should be quite safe :). |
|
Yeah this is a really cool idea @ismay! I'll just take a bit of time to look at this and the repo you created to understand exactly what it is we're doing so I can give better feedback but I'll get back to you ASAP! Also, I was just thinking, since we probably don't want to for example release this one before 1.0 (unless we just publish a 0.4.0 of course) we could make a 1.0 branch now that we merge this and other breaking stuff like my PR into until we're ready to publish and then merge it into master? |
Cool thanks!
Yeah sounds like a good idea 👍 |
Yeah I was getting that as well, it was so annoying, thanks for updating it! I don't think it should be a problem doing it in this PR though it could of course be seen as cleaner doing it in a different one.
Yeah why is it it's a dependency by the way? It shouldn't need to be a peerDependency either should it? As in, if you don't use Typescript you shouldn't need it, couldn't we just make it a devDependency since the only reason we have it here is for testing, right? Or am I missing something? |
I think we did that because typescript-eslint-parser throws if typescript can't be found (or complained anyway). Which is annoying for people who don't use our processor for typescript as they have no need for typescript in their project. So we're basically stuck with this situation unless it can somehow be fixed on typescript-eslint-parser's end (by not doing the check until it's actually instantiated or something, don't know what they're doing exactly). |
Hmm that's a pain <.<. When I'm done reviewing this which should be in a sec I'll just look at our code regarding Typescript as I have never looked at that, mostly out of curiosity, would be awesome if we could remove that dependency somehow for sure though it doesn't sound like it. |
There was a problem hiding this comment.
This review is purely things I just noticed now that has nothing to do with your PR :P. I'd love responses to this, and if they incur changes they should definitely not be handled in this PR, but in others and we can open an issue or something. But yeah I'll do a real review now ^^.
src/index.js
Outdated
| @@ -1,16 +1,6 @@ | |||
| const path = require('path') | |||
| const parse = require('./parsers/index') | |||
| // TODO Fix ampersand in selectors | |||
There was a problem hiding this comment.
What is it we mean here? Is this still a TODO?
There was a problem hiding this comment.
Yes I was also curious about that. @mxstbr Can we remove this? Was this related to the stylelint rules we wanted to enforce?
There was a problem hiding this comment.
No idea anymore haha, let's get rid of it!
src/index.js
Outdated
| if (ignoredRules.includes(warning.rule)) return prevWarnings | ||
| const correctedWarning = Object.assign(warning, { | ||
| const warnings = stylelintResult.warnings.map(warning => | ||
| Object.assign(warning, { | ||
| // Replace "brace" with "backtick" in warnings, e.g. | ||
| // "Unexpected empty line before closing backtick" (instead of "brace") |
There was a problem hiding this comment.
This wouldn't be correct if it you had a declaration block inside of your backticks, right? Maybe we should do some check and only do that replace if it's on line 0 or the last line of the block?
There was a problem hiding this comment.
Yeah there's definitely a lot more stuff we could/should do with the warnings in the future! Could get a bit smarter 😊
There was a problem hiding this comment.
Okay I'll open a quick issue to keep it on our radar, but it's definitely not high prio :)
There was a problem hiding this comment.
I know this isn't something new you introduced in the PR but something that was already there, but it's a small change and unless you have a good reason for keeping it I'd prefer changing it?
Otherwise this looks great, and also the other repo, I really like this idea, so much cleaner, and gives the user some more freedom and flexibility. Really nice initiative :D
src/index.js
Outdated
| if (ignoredRules.includes(warning.rule)) return prevWarnings | ||
| const correctedWarning = Object.assign(warning, { | ||
| const warnings = stylelintResult.warnings.map(warning => | ||
| Object.assign(warning, { |
There was a problem hiding this comment.
Hmm for good form I might not mutate warning and do Object.assign({}, warning, {. Also see my comment on line 32 below.
src/index.js
Outdated
|
|
||
| return Object.assign(stylelintResult, { warnings: newWarnings }) | ||
| return Object.assign(stylelintResult, { warnings }) |
There was a problem hiding this comment.
As we currently mutate warning in the .map above this is actually unnecessary right? I think with current mutations this is actually the same as just returning stylelintResult. I understand that's not as clear though, so maybe I'd recommend just not mutating and following my suggestion above and then also not mutating here? As in again add a first argument {}.
There was a problem hiding this comment.
You're right, fixed it as per your suggestions
|
I just created a 1.0 branch and change the base, I'll go change the base of my PR now as well :) |
|
I've created #99 btw, as a reminder of the typescript dependency discussion. |
Yep, and I'll doublecheck whether the shareable config has been published on npm. |
|
@emilgoldsmith Ready to be merged, if you agree on the readme updates. |
|
Looks good to me! I'm a big fan of this PR, let's merge it in :D |
Closes #45
Closes #9
This'll be a breaking change. It can break builds because we're now prohibiting the use of vendor-prefixed properties and values, and a failure to include the config as a dependency could potentially break builds as well (unfixable rules won't be ignored anymore).