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

1.7.1 Release #2887

Closed
suchipi opened this issue Sep 25, 2017 · 28 comments
Closed

1.7.1 Release #2887

suchipi opened this issue Sep 25, 2017 · 28 comments
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.

Comments

@suchipi
Copy link
Member

suchipi commented Sep 25, 2017

There have been a lot of CSS lowercase fixes and we are starting to get duplicate reports on some of them. We should probably publish a patch release containing those fixes.

@vjeux
Copy link
Contributor

vjeux commented Sep 25, 2017

Sgtm

@ikatyang
Copy link
Member

ikatyang commented Sep 25, 2017

Notice that we should uncomment 68d1b56 before releasing.

  • uncomment docs for config file extensions

@lydell
Copy link
Member

lydell commented Sep 25, 2017

Any objections to merging #2844 before 1.7.1?

@suchipi
Copy link
Member Author

suchipi commented Sep 25, 2017

I think the only potential blocker on that would be the discussion in #2846, since the CLI arguments are part of the public API, so if we released #2844 and then changed the parser option, it'd be a breaking change.

@suchipi
Copy link
Member Author

suchipi commented Sep 25, 2017

To clarify... #2846 would be a breaking change, too. I'm concerned about --parser postcss-less changing into --parser less

@vjeux
Copy link
Contributor

vjeux commented Sep 26, 2017

By the way, I just want to mention that I'm heads down on my new team and won't have the time to be as involved in the next few months. I just clicked the "Unwatch" button, please cc me if anything needs my attention.

I trust @azz, @lydell, @suchipi and co to make the project move forward in a good way :)

@mitermayer
Copy link
Member

Hi guys,

What are your thoughts around including the prettier version on error messages on the CLI ?

This could give users of editor integrations a bit more context on why things may break, which is important since we do CLI args validations.

The current model of CLI args validation forces any editor integration that rely on the cli to do major releases (breaking changes) since by passing the new args it completely break compatibility with older prettier.

The reason I raised this discussion in here is due to the work that has been done around validation of CLI args to 1.7.1

@lydell
Copy link
Member

lydell commented Sep 26, 2017

@mitermayer Could you give an example? Prettier only warns about unknown options, so passing new options to an old Prettier should not be a problem? What work around the CLI are you talking about specifically?

@ikatyang
Copy link
Member

@mitermayer

We didn't add additional validations for CLI, just add missing validations for configs from --config-precedence option.

New flags will be always ignored and show warnings, so it shouldn't be a problem, the only breakings will be new choices since the old version didn't know what to do with it, e.g. --parser <css|less|scss>, but I don't know if we can do something for it.

@mitermayer
Copy link
Member

The problem happens because, when prettier release new features we want to add support to it on editors. We do that by passing extra arguments to the cli. And the most common form of editor integration is by leveraging "stdin" and parsing the values of "stdout".

The error 'unknown arg blah' can cause things like this: prettier/vim-prettier#53

Another possible solution is if there was a way to mute this errors from being printed when executing the CLI

@suchipi
Copy link
Member Author

suchipi commented Sep 26, 2017

Maybe when an error occurs, you could run prettier -v and warn the user if it's not the expected version?

@suchipi
Copy link
Member Author

suchipi commented Sep 26, 2017

Unless there's something I don't understand, I'm not sure what the difference between that and including the version number in the error messages would be.

@lydell
Copy link
Member

lydell commented Sep 26, 2017

@mitermayer Can't you hide unwanted warnings?

@mitermayer
Copy link
Member

@suchipi, running prettier again to get the version is an option but there are some problems with it lets imagine this scenario

  1. user saves file and then we use this new extra 'arg' to pass to prettier
  2. prettier does not understand that 'arg' and print that message
  3. on the editor integration we treat any stdout that is not the file itself as errors
  4. we now have to do another system call to prettier to get the version (which is expensive)

The above logic is not ideal for editor integrations for 2 reasons, have an extra version call to the client (which could in theory also fail) and also means that editor integrations would have to keep up to date on how prettier spits errors on the stdout in order to parse it correctly. (we already do that by handling syntax errors, having other types of errors like that just increase complexity)

My suggestion is if we could have either:

  1. A fixed place where the prettier version would always be printed alongside any error message (we could then regex parse it) and any changes around it should be considered major release

or

  1. An extra argument that we can pass to the cli that would allow the validation args message to not be printed on 'stdout' example that 'unknown arg'

What are your thouhts around it ?

@ikatyang
Copy link
Member

The warnings/errors are always printed to stderr, only valid output will be printed to stdout, and you can also catch the exit code to determine if it's an error.

Or maybe we can allow --version not to early exit.

@lydell
Copy link
Member

lydell commented Sep 26, 2017

Another possible solution is if there was a way to mute this errors from being printed when executing the CLI

Redirect stderr to /dev/null

@suchipi
Copy link
Member Author

suchipi commented Sep 26, 2017

@ikatyang so you're saying you could include --version with any run, and it would print version, and then do whatever else you asked for?

@ikatyang
Copy link
Member

@suchipi Just like what jest --debug did, but I'm not sure if we should do that.

@suchipi
Copy link
Member Author

suchipi commented Sep 26, 2017

@mitermayer Could we move this conversation to a new issue? I don't think it needs to block a patch/bugfix release.

@mitermayer
Copy link
Member

@suchipi sounds good to me!

@lydell
Copy link
Member

lydell commented Sep 26, 2017

@suchipi Are you up for writing the change log again? Or should we look if anyone else is interested?

@suchipi
Copy link
Member Author

suchipi commented Sep 26, 2017

I can do it. Since it's just a point release, I was thinking that something like what we did for 1.6.1 would be enough, rather than a dedicated release document (like we did for 1.6.0 and 1.7.0). But I haven't reviewed all the merged PRs yet, so if there's enough stuff, maybe we should write a larger changelog doc.

My hope is to get the changelog done this evening and then publish the new release tomorrow morning. I could publish it in the evening, too, but last time around we published in the morning so that stuff wouldn't break while people were asleep; I think it'd be good to do it that way again.

@suchipi
Copy link
Member Author

suchipi commented Sep 27, 2017

Okay, here's a proposed changelog and list of all the TODOs we left for ourselves to take care of post->1.7.0

Commits: 1.7.0...master

Changelog (based on this PR search):

Prerelease action item: uncomment this

  • README.md#L702 (Github doesn't let me link to a line in a markdown file, because there's no source preview for markdown files)

Postrelease action items (things that need to be removed/uncommented):

  • <label>--parser <select id="parser"><option value="babylon">babylon</option><option value="flow">flow</option><option value="typescript">typescript</option><option value="postcss">postcss</option><!-- TODO: Enable these and remove the "postcss" option when prettier@>1.7.0 has been released. <option value="css">css</option><option value="less">less</option><option value="scss">scss</option>--><option value="json">json</option><option value="graphql">graphql</option></select></label>
  • // TODO: Remove the "postcss" case when prettier@>1.7.0 is released.
  • // TODO: Remove the "postcss" case when prettier@>1.7.0 is released.
  • // TODO: Remove "postcss" when prettier@>1.7.0 is released.

I was hoping to get the release out in the morning, but I have to go into the doctor in the morning, so it will probably not go out until midday.

@lydell
Copy link
Member

lydell commented Sep 27, 2017

Hmm, actually, some of those Remove "postcss" comments might not be correct. We might want to keep some of them for backwards compatibility. Don't bother with them, and I can look over it once the release is out :)

@suchipi
Copy link
Member Author

suchipi commented Sep 27, 2017

Okay, sounds good 👍 I'll just uncomment the one @ikatyang mentioned then (the yml thing in the README)

@suchipi
Copy link
Member Author

suchipi commented Sep 27, 2017

Released: c2bc33b

@suchipi suchipi closed this as completed Sep 27, 2017
@lydell
Copy link
Member

lydell commented Sep 27, 2017

I've done the postrelease stuff around postcss and the playground now, and it seems to work out.

@suchipi
Copy link
Member Author

suchipi commented Sep 27, 2017

Cool, thank you for doing that @lydell

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests

5 participants