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

Moving the realpath resolution to a function that uses it, so it doesn't fail on lib/sync import #220

Merged
merged 1 commit into from
Apr 17, 2020
Merged

Moving the realpath resolution to a function that uses it, so it doesn't fail on lib/sync import #220

merged 1 commit into from
Apr 17, 2020

Conversation

lgandecki
Copy link
Contributor

535ec22#diff-688ede0ee2da1fa3d89f2fc485f39273R8

This change caused some issues for the users of Cypress Cucumber Preprocessor - badeball/cypress-cucumber-preprocessor#362

putting "resolve": "1.15.1"
in package.json resolutions helps.

I'm guessing we never, directly or indirectly, call the maybeUnwrapSymlink function so having realpath not being defined was never a problem for us.

I'd appreciate a tweak here. I'm also guessing we might not the only ones that will bump into this problem in the future.

@lgandecki lgandecki mentioned this pull request Apr 16, 2020
Closed
@tim-yao tim-yao mentioned this pull request Apr 17, 2020
Closed
@ljharb
Copy link
Member

ljharb commented Apr 17, 2020

In resolve v2, preserveSymlinks will be on by default.

In general, if you mock out a node core module, and you don't do it correctly, then lots of things could break.

The solution is to fix cypress, not to alter this module.

That said, it's certainly possible to use resolve without a filesystem, so we shouldn't crash when someone provides a broken fs module.

#218 should help with this as well.

ljharb
ljharb requested changes Apr 17, 2020
View reviewed changes
lib/async.js Show resolved Hide resolved
ljharb
ljharb approved these changes Apr 17, 2020
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

i updated the commit to avoid the crash without making resolve brittle.

Again, if fs exists but fs.realpath/fs.realpathSync doesn't, your fs implementation is broken. It's not the responsibility of the ecosystem to absorb the effects of your incomplete mock.

@lgandecki
Copy link
Contributor Author

Makes sense! Thanks for taking your time to look into this.

@ljharb ljharb merged commit bf68cbd into browserify:master Apr 17, 2020
ljharb pushed a commit that referenced this pull request Apr 17, 2020
@lgandecki @ljharb
[patch] when a non-node fs is broken and lacks realpath/`realpath…
782dfec 
…Sync`, do not crash (#220)
ljharb added a commit that referenced this pull request Apr 17, 2020
@ljharb
v1.16.1
ac3eb2f 
 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)
ljharb added a commit that referenced this pull request Apr 17, 2020
@ljharb
Merge tag 'v1.16.1'
8238b8c 
v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)
ljharb added a commit that referenced this pull request Oct 21, 2020
@ljharb
v2.0.0-next.2
9a318c3 
Changes since v2.0.0-next.1:

 - [Breaking] remove `core`/`isCore` in favor of `is-core-module` package
 - [Fix] drop the "require" condition, since that causes an "experimental" warning to be printed (#209)

Including all changes in v1.15.1 - v1.18.1:

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
ljharb added a commit to ljharb/resolve that referenced this pull request Feb 11, 2021
@ljharb
v2.0.0-next.3
6674173 
Changes since v2.0.0-next.2:

 - [New] add `readPackage` and `readPackageSync` (browserify#236)
 - [Fix] throw an error for an invalid explicit `main` with a missing `./index.js` file (browserify#234)
 - [meta] create SECURITY.md
 - [Deps] update `is-core-module`
 - [Dev Deps] update `eslint`, `@ljharb/eslitn-config`, `array.prototype.map`, `aud`, `tape`

Including all changes in v1.15.1 - v1.19.0:

v1.19.0

 - [New] `sync`/`async`: add 'includeCoreModules' option (browserify#233)
 - [readme] Add possible error types (browserify#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (browserify#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (browserify#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (browserify#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (browserify#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
ljharb added a commit that referenced this pull request Feb 11, 2021
@ljharb
v2.0.0-next.3
c6bf2a5 
Changes since v2.0.0-next.2:

 - [New] add `readPackage` and `readPackageSync` (#236)
 - [Fix] throw an error for an invalid explicit `main` with a missing `./index.js` file (#234)
 - [meta] create SECURITY.md
 - [meta] do not publish github action workflow files
 - [Deps] update `is-core-module`
 - [Dev Deps] update `eslint`, `@ljharb/eslitn-config`, `array.prototype.map`, `aud`, `tape`

Including all changes in v1.15.1 - v1.19.0:

v1.19.0

 - [New] `sync`/`async`: add 'includeCoreModules' option (#233)
 - [readme] Add possible error types (#232)
 - [Deps] update `is-core-module`
 - [Dev Deps] update `aud`, `eslint`
 - [meta] add Automatic Rebase and Require Allow Edits workflows
 - [Tests] comment out node 15 in appveyor; it’s not available yet
 - [Tests] add node 15 to appveyor, fix "latest npm" logic
 - [Tests] migrate tests to Github Actions

v1.18.1

 - [Fix] `core`: remove console warning on require, since the main entry point requires it
 - [Refactor] avoid using extensions in require paths
 - [meta] update auto-generated `core.json`
 - [meta] add `eclint` and `.editorconfig`
 - [Dev Deps] update `eslint`
 - [Dev Deps] add `aud` in `posttest`

v1.18.0

 - [New] extract `isCore` implementation to `is-core-module`
 - [New] add new core modules that will be in node v15
 - [readme] soft-deprecate `resolve.isCore`
 - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape`

v1.17.0

 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (#218)
 - [Dev Deps] update `tape`

v1.16.1

 - [patch] when a non-node `fs` is broken and lacks `realpath`/`realpathSync`, do not crash (#220)

v1.16.0

 - [New] `core`: `fs/promises` is a core module again in node 14+ (f6473e2)
 - [patch] `sync`/`async`: use native `realpath` if available to unwrap symlinks (#217)

v1.15.1

 - [Fix] correct behavior when requiring `.` with same name (#212)
 - [Dev Deps] update `@ljharb/eslint-config`
 - [Tests] allow node 5 on windows to fail due to npm registry bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

@0gust1 0gust1 0gust1 approved these changes

@zciluk zciluk zciluk approved these changes

@alexandrerousselledecath alexandrerousselledecath alexandrerousselledecath approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lgandecki @ljharb @0gust1 @zciluk @alexandrerousselledecath