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

[New] sync/async: add realpath/realpathSync options #218

Merged
merged 1 commit into from
Apr 22, 2020
Merged

[New] sync/async: add realpath/realpathSync options #218

merged 1 commit into from
Apr 22, 2020

Conversation

SimenB
Copy link
Contributor

@SimenB SimenB commented Apr 7, 2020 •
edited
Loading

Not sure how to write tests for this one. Ideas?

I also went with passing the implementation to maybeUnwrap so consumer wouldn't have to care about the option, not sure if it's a good idea or not.

(This is probably the one we want in Jest so we can plug in the hacky process.bindings one on node 8...)

@ljharb
Copy link
Member

ljharb commented Apr 8, 2020

I think i'd write a sync + async test that would ordinarily fail tests, but passes because you overrode unwrapSymlink?

ljharb
ljharb reviewed Apr 8, 2020
View reviewed changes
lib/sync.js Outdated Show resolved Hide resolved
lib/async.js Outdated Show resolved Hide resolved
@SimenB
Copy link
Contributor Author

SimenB commented Apr 8, 2020

I think i'd write a sync + async test that would ordinarily fail tests, but passes because you overrode unwrapSymlink?

Sounds good. Can I make it not actually do any symlink stuff but just pretend it does? E.g. add an extra directory after "resolving" the symlink?

@SimenB
Copy link
Contributor Author

SimenB commented Apr 8, 2020

Rebased after #217, will write a test after breakfast 🙂

@SimenB
Copy link
Contributor Author

SimenB commented Apr 8, 2020

@ljharb pushed some tests, PTAL

@SimenB SimenB mentioned this pull request Apr 8, 2020
Closed
SimenB
SimenB commented Apr 8, 2020
View reviewed changes
readme.markdown Outdated
@@ -117,6 +119,13 @@ default `opts` values:
return cb(err);
});
},
unwrapSymlink: function unwrapSymlink(file, cb) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not too fond of this name - I just copied the name from the code. Should we call it realpath and realpathSync to mirror readFile and readFileSync options?

Copy link
Member

Choose a reason for hiding this comment

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

i don't have a strong opinion; "realpath" is definitely the typical name for this, and "unwrap" really is just a word that makes sense in my head, not something that actually makes sense :-)

I'll update this to match your suggestions.

@ljharb ljharb mentioned this pull request Apr 17, 2020
Merged
@SimenB
Copy link
Contributor Author

SimenB commented Apr 22, 2020

@ljharb thoughts on this?

ljharb
ljharb approved these changes Apr 22, 2020
View reviewed changes
readme.markdown Outdated
@@ -117,6 +119,13 @@ default `opts` values:
return cb(err);
});
},
unwrapSymlink: function unwrapSymlink(file, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

i don't have a strong opinion; "realpath" is definitely the typical name for this, and "unwrap" really is just a word that makes sense in my head, not something that actually makes sense :-)

I'll update this to match your suggestions.

@ljharb ljharb changed the title feat: make unwrapSymlink pluggable [New] sync/async: add realpath/realpathSync options Apr 22, 2020
@ljharb ljharb merged commit be951d3 into browserify:master Apr 22, 2020
ljharb pushed a commit to ljharb/resolve that referenced this pull request Apr 22, 2020
@SimenB @ljharb
[New] sync/async: add realpath/realpathSync options (browseri…
52348f1 
…fy#218)
ljharb added a commit to ljharb/resolve that referenced this pull request Apr 22, 2020
@ljharb
v1.17.0
3a76ef8 
 - [New] `sync`/`async`: add `realpath`/`realpathSync` options (browserify#218)
 - [Dev Deps] update `tape`
ljharb added a commit that referenced this pull request Apr 22, 2020
@ljharb
Merge tag 'v1.17.0'
944728d 
v1.17.0

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

ljharb commented Apr 22, 2020

Released in v1.17.0.

@SimenB SimenB deleted the pluggable-unwrap-symlink branch April 23, 2020 06:37
@SimenB
Copy link
Contributor Author

SimenB commented Apr 23, 2020

Thank you!

@SimenB SimenB mentioned this pull request Apr 23, 2020
Merged
10 tasks
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

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SimenB @ljharb