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

Use resolve if require.resolve is overridden #8072

Merged
merged 14 commits into from Apr 21, 2020
Merged

Use resolve if require.resolve is overridden #8072

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c00689d
Use `resolve` insteadof `require.resolve`
fisker Apr 17, 2020
157e90e
Use `resolve` as fallback
fisker Apr 17, 2020
d06c42e
Lazy load
fisker Apr 17, 2020
8ab88f4
Update resolve.js
fisker Apr 17, 2020
cc574bb
Disable length check to run tests
fisker Apr 17, 2020
34d881e
Merge branch 'master' into pr-7508-revert
fisker Apr 17, 2020
eec17f5
Try to test
fisker Apr 17, 2020
b35f7cf
istanbul ignore
fisker Apr 17, 2020
e13a469
Comment
fisker Apr 17, 2020
bf56032
Fix argument
fisker Apr 17, 2020
0f0e8ce
Try upload coverage separately
fisker Apr 18, 2020
7d741b2
Update src/common/resolve.js
thorn0 Apr 20, 2020
61b3d4d
add eslint-disable
thorn0 Apr 20, 2020
a5950e8
add comment
thorn0 Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions .github/workflows/dev-test.yml
View file
Open in desktop
Expand Up @@ -67,3 +67,17 @@ jobs:
token: ${{ secrets.CODECOV_TOKEN }}
file: ./coverage/lcov.info
fail_ci_if_error: true

# #8073 test
- name: Run Tests (PRETTIER_FALLBACK_RESOLVE)
run: yarn test "tests_integration/__tests__/(config|plugin)"
env:
PRETTIER_FALLBACK_RESOLVE: true

- name: Upload Coverage (PRETTIER_FALLBACK_RESOLVE)
uses: codecov/codecov-action@v1
if: matrix.ENABLE_CODE_COVERAGE
with:
token: ${{ secrets.CODECOV_TOKEN }}
file: ./coverage/lcov.info
fail_ci_if_error: true
7 changes: 7 additions & 0 deletions .github/workflows/prod-test.yml
View file
Open in desktop
Expand Up @@ -131,3 +131,10 @@ jobs:
- name: Run Tests (standalone) (Linux and Windows)
if: matrix.os != 'macos-latest'
run: yarn test:dist-standalone --maxWorkers=2

# #8073 test
- name: Run Tests (PRETTIER_FALLBACK_RESOLVE)
fisker marked this conversation as resolved.
Show resolved Hide resolved
run: yarn test "tests_integration/__tests__/(config|plugin)"
env:
NODE_ENV: production
PRETTIER_FALLBACK_RESOLVE: true
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -62,6 +62,7 @@
"regexp-util": "1.2.2",
"remark-math": "1.0.6",
"remark-parse": "5.0.0",
"resolve": "1.16.0",
"semver": "7.3.2",
"srcset": "2.0.1",
"string-width": "4.2.0",
Expand Down
15 changes: 10 additions & 5 deletions src/common/resolve.js
View file
Open in desktop
Expand Up @@ -2,11 +2,16 @@

let { resolve } = eval("require");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let { resolve } = eval("require");
let resolve = eval("require").resolve;

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I've tried this, won't work , this will replaced by babel https://github.com/prettier/prettier/blob/master/scripts/build/babel-plugins/transform-custom-require.js
to require.resolve , then rollup replace it to commonJsRequire.resolve

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

I'm not sure, but global.require.resolve seems a good solution.

Copy link
Member

Choose a reason for hiding this comment

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

Are you sure? I tested it too locally and it stays this way in dist/index.js:

let resolve$1 = require.revolve; // In the VS Code and Atom extension `require` is overridden and `require.resolve` doesn't support the 2nd argument.

if (resolve$1.length === 1 || process.env.PRETTIER_FALLBACK_RESOLVE) {

Copy link
Member

Choose a reason for hiding this comment

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

global.require exists only in the REPL.

Copy link
Member

Choose a reason for hiding this comment

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

Strangely. locally ESLint doesn't issue a warning for this line for me. 🤔

Copy link
Sponsor Member Author

@fisker fisker Apr 20, 2020 •
edited

Choose a reason for hiding this comment

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

Hmm, I guess I tried let { resolve } = require; andlet resolve = require.resolve;.

Eslint didn't warn on my machine either, we need add comments and disable the rule on it anyway.


// In the VS Code extension `require` is overridden and `require.resolve` doesn't support the 2nd argument.
if (resolve.length === 1) {
const Module = eval("require")("module");
const createRequire = Module.createRequire || Module.createRequireFromPath;
resolve = createRequire(__dirname).resolve;
// In the VS Code and Atom extension `require` is overridden and `require.resolve` doesn't support the 2nd argument.
if (resolve.length === 1 || process.env.PRETTIER_FALLBACK_RESOLVE) {
resolve = (id, options) => {
let basedir;
if (options && options.paths && options.paths.length === 1) {
basedir = options.paths[0];
}

return require("resolve").sync(id, { basedir });
};
}

module.exports = resolve;
7 changes: 7 additions & 0 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -6451,6 +6451,13 @@ resolve@1.1.7:
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.1.7.tgz#203114d82ad2c5ed9e8e0411b3932875e889e97b"
integrity sha1-IDEU2CrSxe2ejgQRs5ModeiJ6Xs=

resolve@1.16.0:
version "1.16.0"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.16.0.tgz#063dc704fa3413e13ac1d0d1756a7cbfe95dd1a7"
integrity sha512-LarL/PIKJvc09k1jaeT4kQb/8/7P+qV4qSnN2K80AES+OHdfZELAKVOBjxsvtToT/uLOfFbvYvKfZmV8cee7nA==
dependencies:
path-parse "^1.0.6"

resolve@^1.1.6, resolve@^1.10.0, resolve@^1.11.0, resolve@^1.12.0, resolve@^1.13.1, resolve@^1.14.2, resolve@^1.15.1, resolve@^1.3.2:
version "1.15.1"
resolved "https://registry.yarnpkg.com/resolve/-/resolve-1.15.1.tgz#27bdcdeffeaf2d6244b95bb0f9f4b4653451f3e8"
Expand Down