Skip to content

Lift requires#3780

Merged
cpojer merged 6 commits intojestjs:masterfrom
SimenB:lift-requires
Jun 10, 2017
Merged

Lift requires#3780
cpojer merged 6 commits intojestjs:masterfrom
SimenB:lift-requires

Conversation

@SimenB
Copy link
Member

@SimenB SimenB commented Jun 9, 2017

Summary
Move a lot of the changes in #3493 to isolate the changes causing errors.

Test plan
Tests should pass

@SimenB
Copy link
Member Author

SimenB commented Jun 9, 2017

This fails locally for some reason.

 FAIL  packages/jest-repl/src/__tests__/jest-repl-test.js                                                                                                                                                                                [2/2]
  ● Repl › cli › runs without errors

    expect(received).toMatch(expected)

    Expected value to match:
      /›/
    Received:
      ""

      at Object.it (packages/jest-repl/src/__tests__/jest-repl-test.js:28:36)
          at Promise (<anonymous>)
          at <anonymous>
      at process._tickCallback (internal/process/next_tick.js:169:7)

  Repl
    cli
      ✕ runs without errors (513ms)

SimenB
SimenB commented Jun 9, 2017
View reviewed changes
packages/jest-cli/src/cli/getJest.js
/* $FlowFixMe */
return require(binPath);
} else {
// TODO: Because of a dependency cycle, this can only be inlined once the babel plugin for inlining is merged
Copy link
Member Author

@SimenB SimenB Jun 9, 2017

Choose a reason for hiding this comment

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

This should almost be considered a bug, IMO

@SimenB SimenB mentioned this pull request Jun 9, 2017
Closed
SimenB
SimenB commented Jun 9, 2017
View reviewed changes
packages/jest-resolve/src/index.js
const resolver = options.resolver
/* $FlowFixMe */
? require(options.resolver)
? /* $FlowFixMe */
Copy link
Member Author

@SimenB SimenB Jun 9, 2017

Choose a reason for hiding this comment

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

Not a fan of this one, /cc @vjeux

Copy link
Contributor

@vjeux vjeux Jun 9, 2017

Choose a reason for hiding this comment

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

I'm curious, is this a regression from before?

It indeed looked better before.

Copy link
Member Author

@SimenB SimenB Jun 9, 2017

Choose a reason for hiding this comment

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

No, it was in a single line before, it was multilined when I made it into a ternary

Copy link
Contributor

@vjeux vjeux Jun 9, 2017

Choose a reason for hiding this comment

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

Could you open an issue against prettier? We may be able to print it the way you want, we're already doing that for flow unions.

Copy link
Member Author

@SimenB SimenB Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Will do!

Copy link
Member Author

@SimenB SimenB Jun 9, 2017

Choose a reason for hiding this comment

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

prettier/prettier#2076

@SimenB SimenB mentioned this pull request Jun 9, 2017
Open
@codecov-io
Copy link

codecov-io commented Jun 9, 2017

Codecov Report

Merging #3780 into master will decrease coverage by 0.07%.
The diff coverage is 52.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3780      +/-   ##
==========================================
- Coverage   58.09%   58.02%   -0.08%     
==========================================
  Files         191      191              
  Lines        6708     6711       +3     
  Branches        6        6              
==========================================
- Hits         3897     3894       -3     
- Misses       2808     2814       +6     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-cli/src/TestNamePatternPrompt.js 94.11% <ø> (ø) ⬆️
...ages/pretty-format/src/plugins/ImmutablePlugins.js 100% <ø> (ø) ⬆️
...kages/pretty-format/src/plugins/ImmutableRecord.js 100% <ø> (ø) ⬆️
packages/jest-validate/src/utils.js 100% <ø> (ø) ⬆️
packages/eslint-config-fb-strict/index.js 0% <ø> (ø) ⬆️
packages/jest-cli/src/TestRunner.js 25.3% <ø> (+0.15%) ⬆️
packages/eslint-plugin-jest/src/index.js 100% <ø> (ø) ⬆️
packages/pretty-format/src/index.js 98.05% <ø> (ø) ⬆️
packages/jest-changed-files/src/index.js 0% <0%> (ø) ⬆️
packages/jest-validate/src/index.js 0% <0%> (ø) ⬆️
... and 14 more

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 dec54a3...69095fe. Read the comment docs.

SimenB added a commit to SimenB/jest that referenced this pull request Jun 9, 2017
@SimenB
[squash] package up jestjs#3780
56858d6
@cpojer cpojer merged commit e0e170a into jestjs:master Jun 10, 2017
@SimenB SimenB deleted the lift-requires branch June 10, 2017 16:12
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
@SimenB @cpojer
Lift requires (jestjs#3780)
0fa5535 
* Lift all inline requires to the top-level

We want to inline them using a babel plugin

* Remove one inlining that messes us up

* Inline two more requires causing the repl test to fail

* remove one unnecessary inline

* Fix prettier lint

* really prettier?
@github-actions
Copy link

github-actions bot commented May 13, 2021

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 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@vjeux vjeux vjeux left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed ✔️

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@SimenB @codecov-io @vjeux @cpojer @facebook-github-bot