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

Upgrade: pin @babel/code-frame@7.12.11 #14067

Merged
merged 2 commits into from Feb 10, 2021
Merged

Conversation

mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Feb 4, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain:

We're using @babel/code-frame for the codeframe formatter.

The latest @babel/code-frame version 7.12.13, which was released yesterday, is causing failures on our CI:

https://github.com/eslint/eslint/pull/14012/checks?check_run_id=1826678459

Tests are failing because 7.12.13 produces different output for empty lines: babel/babel#12567. That change is intentional. In particular, it removes a trailing space after | if the line is empty.

7.12.13 is in the specified range:

"@babel/code-frame": "^7.0.0",

What changes did you make? (Give an overview)

  • Fixed the failing tests to match the behavior of v7.12.13.

  • Updated "@babel/code-frame": "^7.12.13" in package.json.

Pinned @babel/code-frame to 7.12.11 (that's the previous version, there is no 7.12.12).

Is there anything you'd like reviewers to focus on?

The change in @babel/code-frame changes the output of our codeframe formatter. An alternative is to pin @babel/code-frame to a previous version and upgrade in the next ESLint major. Another alternative is to update the code in the formatter to restore the previous output, if possible.

  • As we probably don't want to stay on 7.12.11 forever, the next ESLint major version should have ^7.12.13 (or higher). That will be a breaking change in the codeframe formatter.
  • An alternative is that we modify the output to match the previous behavior.

@mdjermanovic mdjermanovic added upgrade This change is related to a dependency upgrade evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 4, 2021
@nzakas
Copy link
Member

nzakas commented Feb 4, 2021

It might be better to pin to the previous version. I don’t think we’ve ever made changes to formatter output outside of a major release.

@mdjermanovic mdjermanovic marked this pull request as draft February 4, 2021 17:13
@mdjermanovic mdjermanovic changed the title Upgrade: @babel/code-frame@7.12.13 Upgrade: pin @babel/code-frame@7.12.11 Feb 5, 2021
@mdjermanovic mdjermanovic marked this pull request as ready for review February 5, 2021 02:24
@mdjermanovic
Copy link
Member Author

Pinned to 7.12.11 and updated the PR description.

nzakas
nzakas approved these changes Feb 9, 2021
btmills
btmills approved these changes Feb 9, 2021
@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Feb 10, 2021
@mdjermanovic
Copy link
Member Author

It seems we have a consensus to pin @babel/code-frame for now, so I'm merging this and will open a follow-up issue.

This was referenced Mar 17, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Aug 10, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Aug 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion upgrade This change is related to a dependency upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants