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

Fix for Babel7 bridge package name in documentation. #6745

Merged
merged 6 commits into from
Sep 15, 2018
Merged

Fix for Babel7 bridge package name in documentation. #6745

merged 6 commits into from
Sep 15, 2018

Conversation

quisido
Copy link
Contributor

@quisido quisido commented Jul 23, 2018

resolves #6720

Summary

yarn cannot find a package with semvar ^7.0.0-0. Further, the 7.0.0-beta# builds do not work. The accurate package version is ^7.0.0-bridge.0.
image
(The quotation marks around the package in the documentation also cause an error.)

To Reproduce

yarn add --dev babel-core@^7.0.0-0

Expected behavior

The package babel-core@^7.0.0-bridge.0 should be added.

Issue in @babel/babel-bridge

Test plan

No code changes.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cc @SimenB

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

README.md Outdated
>
> ```bash
> yarn add --dev 'babel-core@^7.0.0-0' @babel/core
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should keep the quotes, see #6746

Copy link
Contributor Author

@quisido quisido Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quotes throw an error.
image

Changing it to match the getting started page (re: #6746) just means the getting started page is wrong too.

Here it is without quotes for comparison:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I got from that Issue link is someone saying "There should be quotes to match the docs," as opposed to "There should be quotes in order for this to work." Let me know if the latter is the case @SimenB. I did not notice evidence that the quotes are needed for this to work; however, I do have evidence that the quotes must not exist for this to work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any update on this? I do not agree with your requested changes @SimenB. I believe leaving the quotes breaks the command, as per the screenshots.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB installing without quotes works just fine for me on latest Yarn.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both works for me, but since we got a PR I guess it doesn't work for everybody. Is it a Windows thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SimenB I am on Windows, so that may be the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Babel has accepted this change in their official documentation (see babel/babel-bridge/pull/6). Makes sense to me that Jest should as well.

@codecov-io
Copy link

Codecov Report

Merging #6745 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6745   +/-   ##
=======================================
  Coverage   63.68%   63.68%           
=======================================
  Files         235      235           
  Lines        9007     9007           
  Branches        3        4    +1     
=======================================
  Hits         5736     5736           
  Misses       3270     3270           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61b4342...4c61ef3. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Aug 30, 2018

@quisido
Copy link
Contributor Author

quisido commented Aug 30, 2018

Update GettingStarted and CHANGELOG, as per request. README change was a part of the initial PR.

@DBosley
Copy link

DBosley commented Sep 10, 2018

This documentation is not accurate. The suggestion to use the bridge version of babel-core does not work with Jest because there are hard dependencies on 6.26.3 in jest-config and jest-runtime. Yarn users can set up a resolves section in their package.json, but npm users have to use a hacky rimraf post-install script to remove the old versions or jest flat out doesn't work.

See #6913 for reference.

@quisido
Copy link
Contributor Author

quisido commented Sep 10, 2018

This documentation doesn't change anything from the existing documentation. It is essentially a typo fix. If there is an error in the existing documentation, it should probably be a different PR. That said, I have had no issue using babel-bridge with jest, as per the existing documentation. That is how I encountered this bug when installing the bridge.

Charles Stover and others added 6 commits September 15, 2018 17:00
Removed single quotes from around the package name and improved grammar. The PR did not update from pushing the change to original branch.
@SimenB SimenB merged commit 8b1be33 into jestjs:master Sep 15, 2018
@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Forgot about this, sorry. I updated the versioned docs and merged, thanks!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

babel-core@^7.0.0-0 should be ^7.0.0-bridge.0
6 participants