[New] `sync`/`async`: add `realpath`/`realpathSync` options by SimenB · Pull Request #218 · browserify/resolve

SimenB · 2020-04-07T22:57:22Z

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 · 2020-04-08T07:01:10Z

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

lib/sync.js

lib/async.js

SimenB · 2020-04-08T07:26:39Z

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 · 2020-04-08T07:30:58Z

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

SimenB · 2020-04-08T08:25:01Z

@ljharb pushed some tests, PTAL

SimenB · 2020-04-08T11:21:23Z

readme.markdown

            return cb(err);
        });
    },
+    unwrapSymlink: function unwrapSymlink(file, cb) {


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?

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.

SimenB · 2020-04-22T08:56:32Z

@ljharb thoughts on this?

ljharb · 2020-04-22T19:22:26Z

readme.markdown

            return cb(err);
        });
    },
+    unwrapSymlink: function unwrapSymlink(file, cb) {


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.

…fy#218)

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

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

ljharb · 2020-04-22T22:59:18Z

Released in v1.17.0.

SimenB · 2020-04-23T06:37:57Z

Thank you!

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

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

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

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

be951d3

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 force-pushed the pluggable-unwrap-symlink branch from 0cf3b33 to 52038e1 Compare April 8, 2020 07:30

SimenB mentioned this pull request Apr 8, 2020

Performance loss between jest 25.1.0 and 25.2.7 jestjs/jest#9776

Closed

SimenB commented Apr 8, 2020

View reviewed changes

ljharb mentioned this pull request Apr 17, 2020

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

Merged

ljharb approved these changes Apr 22, 2020

View reviewed changes

ljharb force-pushed the pluggable-unwrap-symlink branch from 894add3 to be951d3 Compare April 22, 2020 19:31

ljharb changed the title ~~feat: make unwrapSymlink pluggable~~ [New] sync/async: add realpath/realpathSync options Apr 22, 2020

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

[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

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

Merge tag 'v1.17.0'

944728d

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

SimenB deleted the pluggable-unwrap-symlink branch April 23, 2020 06:37

SimenB mentioned this pull request Apr 23, 2020

[resolve]: update to v1.17 DefinitelyTyped/DefinitelyTyped#44137

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

[New] `sync`/`async`: add `realpath`/`realpathSync` options#218
ljharb merged 1 commit intobrowserify:masterfrom
SimenB:pluggable-unwrap-symlink

SimenB commented Apr 7, 2020 •

edited

Loading

Uh oh!

ljharb commented Apr 8, 2020

Uh oh!

Uh oh!

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB Apr 8, 2020

Uh oh!

ljharb Apr 22, 2020

Uh oh!

SimenB commented Apr 22, 2020

Uh oh!

ljharb Apr 22, 2020

Uh oh!

ljharb commented Apr 22, 2020

Uh oh!

SimenB commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

SimenB commented Apr 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Apr 8, 2020

Uh oh!

Uh oh!

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

SimenB Apr 8, 2020

Choose a reason for hiding this comment

Uh oh!

ljharb Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

SimenB commented Apr 22, 2020

Uh oh!

ljharb Apr 22, 2020

Choose a reason for hiding this comment

Uh oh!

ljharb commented Apr 22, 2020

Uh oh!

SimenB commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

SimenB commented Apr 7, 2020 •

edited

Loading