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

NPM Push for new error style #26

Closed
Kingdutch opened this issue Mar 24, 2015 · 23 comments
Closed

NPM Push for new error style #26

Kingdutch opened this issue Mar 24, 2015 · 23 comments

Comments

@Kingdutch
Copy link

Could you bump the version and do an npm publish? In 0.2.0, the old style of errors is used which causes file names to be omitted from error messages. The new style error messages can probably fix this. It's a commit made a few months ago so it would be awesome if we could use it

🐹

@davidtheclark
Copy link
Contributor

I'd be in favor of a new release after the current issues are resolved. I wonder if it would be a good time to bump major release: since with the recent changes there's an API, having a major release number would be we could more meaningfully indicate with semver when that API has changed.

@necolas
Copy link
Contributor

necolas commented Mar 26, 2015

Before 1.0.0, the expectation is that minor version increases may break the API. I'd think I'd rather we stay at 0.x and iron out any kinks and instability before we're happy for 1.0.0. For example, going to 0.2 gives time for Twitter to use it for a few weeks and for any non-suit projects to try out the linter.

@davidtheclark
Copy link
Contributor

Ok, sounds good.

@Kingdutch
Copy link
Author

A bump to 0.3.0 would probably fit within semver though right? More meaningful error messages seem something worth releasing :)

@necolas
Copy link
Contributor

necolas commented Mar 28, 2015

Ah yes I meant 0.3.

@Kingdutch
Copy link
Author

👍

@davidtheclark
Copy link
Contributor

@necolas If the pending pull requests work out, I don't have any other particular changes in mind, so would be eager for a new release.

@necolas
Copy link
Contributor

necolas commented Apr 25, 2015

let me test master out against our code base before i cut a new release. will do it this working week

@necolas
Copy link
Contributor

necolas commented Apr 28, 2015

i dropped HEAD into our codebase, introduced errors, and it failed to report them. so there is something not right with HEAD.

@simonsmith
Copy link
Collaborator

A 0.3 release for the latest namespace stuff would be great

@necolas
Copy link
Contributor

necolas commented May 2, 2015

sorry my comment suggested the opposite of what is happening. master doesn't report errors properly, which is why i haven't cut a new version

@davidtheclark
Copy link
Contributor

oh, huh ... errors are logging as PostCSS warnings (as discussed in #32 as #36), which means you'd need to be setup to see those (using postcss-log-warnings or something). do you have that in place?

@necolas
Copy link
Contributor

necolas commented May 2, 2015

the docs need to mention that, otherwise the plugin doesn't really do anything for you. so this plugin would be dependent on using 'postcss-log-warnings' too?

@MoOx
Copy link

MoOx commented May 2, 2015

It depends on your workflow I guess. If you use webpack for example, you would prefer getting webpack warnings emitted. But yeah, your runner has to show you the messages.

davidtheclark added a commit that referenced this issue May 2, 2015
@davidtheclark
Copy link
Contributor

I just added a sentence to Readme --- sorry I didn't know that it would just commit automatically without creating PR :)

Also, if it's really important not to rely on a runner or plugin to log the warnings, we could probably add an option to this plugin that will exit the process and print those warnings immediately.

@necolas
Copy link
Contributor

necolas commented May 3, 2015

We currently want the build to fail when the CSS is not properly scoped, otherwise it will end up in master.

The postcss logger plugin worked, but since we're using webpack I end up with 2 sets of warnings – one from postcss, one from webpack. If I use throwError:true, it exits with 1 but you can't tell why. It's not obvious that all the warnings printed before are actually the source of the error.

@MoOx
Copy link

MoOx commented May 3, 2015

@necolas that why the runner need to emit errors via webpack (eg for eslint: https://github.com/MoOx/eslint-loader#errors-and-warning). This will need to be an option added in postcss-loader

@davidtheclark
Copy link
Contributor

I opened #44 to address this.

@necolas
Copy link
Contributor

necolas commented May 6, 2015

Another thing to discuss. master produces warnings for selectors like:

.Button--modifier.Button--anotherModifier {}

We have some code like that in a new app. I think that pattern probably is something to avoid, because there are basically side-effects from including 2 modifiers together (should probably be a single modifier). Just wanted to check if there is any disagreement about that.

Thanks

@davidtheclark
Copy link
Contributor

Yeah, I agree that it would probably be better to use a single modifier.

@davidtheclark
Copy link
Contributor

@necolas Anything else you'd like to see done before a new release?

@davidtheclark
Copy link
Contributor

@necolas Any word on this?

@necolas
Copy link
Contributor

necolas commented May 23, 2015

tried master on our codebase; looked good. published as 0.3.0. thanks for all the great contributions.

@necolas necolas closed this as completed May 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants