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.

Sign up for GitHub

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

Jump to bottom

cleanup from trying out yarn pnp #8175

Merged
merged 1 commit into from
Apr 7, 2019
Merged

cleanup from trying out yarn pnp #8175

merged 1 commit into from
Apr 7, 2019

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Mar 20, 2019

Summary

Trying out Yarn PnP some more again revealed a few things we can do better

Test plan

We'll see if CI passes.

jeysal
jeysal commented Mar 20, 2019
View reviewed changes
babel.config.js Outdated
@@ -4,28 +4,31 @@ module.exports = {
babelrcRoots: ['examples/*'],
overrides: [
{
presets: ['@babel/preset-flow'],
presets: [require.resolve('@babel/preset-flow')],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise Babel would resolve these from e2e dirs that are not Yarn workspaces, where they are not installed as deps

jeysal
jeysal commented Mar 20, 2019
View reviewed changes
examples/react-native/__tests__/__snapshots__/intro.test.js.snap Outdated
@@ -84,6 +84,7 @@ exports[`renders the TextInput component 1`] = `
<TextInput
allowFontScaling={true}
autoCorrect={false}
rejectResponderTermination={true}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

From react-native update, @orta said thisisfine.jpeg

jeysal
jeysal commented Mar 20, 2019
View reviewed changes
packages/expect/package.json
@@ -19,7 +19,8 @@
"jest-regex-util": "^24.3.0"
},
"devDependencies": {
"@types/ansi-styles": "^3.2.1"
"@types/ansi-styles": "^3.2.1",
"immutable": "^4.0.0-rc.12"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/facebook/jest/blob/b37a1173543b58c9524c3f23183d2a2dff196bce/packages/expect/src/__tests__/matchers.test.js#L11

jeysal
jeysal commented Mar 20, 2019
View reviewed changes
packages/jest-snapshot/package.json Outdated
@@ -27,6 +27,7 @@
"jest-haste-map": "^24.0.0"
},
"devDependencies": {
"@babel/traverse": "^7.4.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/facebook/jest/blob/b37a1173543b58c9524c3f23183d2a2dff196bce/packages/jest-snapshot/src/__tests__/inline_snapshots.test.ts#L14

@jeysal jeysal changed the title cleanup from trying out yarn pnp WIP DO NOT MERGE cleanup from trying out yarn pnp Mar 20, 2019
@jeysal
Copy link
Contributor Author

jeysal commented Mar 20, 2019

Using require.resolve() for plugins / presets in babel.config.js resulted in super obscure error in places you'd never expect them. No idea what exactly is going on, I removed that part of the cleanup for now.

@jeysal jeysal changed the title WIP DO NOT MERGE cleanup from trying out yarn pnp cleanup from trying out yarn pnp Mar 20, 2019
@codecov-io
Copy link

codecov-io commented Mar 22, 2019 •
edited

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #8175   +/-   ##
======================================
  Coverage    62.3%   62.3%           
======================================
  Files         265     265           
  Lines       10473   10473           
  Branches     2542    2541    -1     
======================================
  Hits         6525    6525           
  Misses       3366    3366           
  Partials      582     582

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 7e4e170...1c310de. Read the comment docs.

@jeysal jeysal requested a review from SimenB March 24, 2019 20:19
SimenB
SimenB approved these changes Mar 26, 2019
View reviewed changes
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.

There's a bunch of extra stuff in node_modules now, maybe not an issue? Seems less than ideal, though

yarn.lock Outdated
dependencies:
uglify-es "^3.1.9"

metro-react-native-babel-preset@*, metro-react-native-babel-preset@0.49.2:
metro-react-native-babel-preset@*:
Copy link
Member

Choose a reason for hiding this comment

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

This is now duplicated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated react-native once more, but that doesn't change it. Upgrading metro-react-native-babel-preset also updates Babel to 7.4.0, so 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind had to revert even the react-native update because it updated Babel. I don't really want to spend more time optimizing resolutions, it's super annoying while our Babel dev dependency ranges include "broken" versions.

@jeysal jeysal force-pushed the cleanup-from-pnp branch 2 times, most recently from 645cf18 to 45df10a Compare March 30, 2019 15:15
@jeysal
Copy link
Contributor Author

jeysal commented Apr 7, 2019

I've removed the problematic parts (babel config require.resolve and react-native upgrade) for now until we have the Babel upgrade issue sorted out

@jeysal jeysal merged commit 8c92898 into jestjs:master Apr 7, 2019
@jeysal jeysal deleted the cleanup-from-pnp branch April 7, 2019 17:57
@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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thymikee thymikee thymikee approved these changes

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jeysal @codecov-io @SimenB @thymikee @facebook-github-bot