feat: use native `realpath` if available to unwrap symlinks by SimenB · Pull Request #217 · browserify/resolve

SimenB · 2020-04-07T21:53:29Z

Docs: https://nodejs.org/api/fs.html#fs_fs_realpath_native_path_options_callback

didn't bother with process.bindings as we really shouldn't use those

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

As an aside, it might make sense to make maybeUnwrapSymlink pluggable like readFile, isFile and isDirectory is already. Those 3 allow you to avoid the real fs if necessary, but the realpath call does not. I can open up a separate issue for that?

ljharb · 2020-04-07T22:22:35Z

A separate issue (or even a PR) sounds good.

ljharb

Assuming tests pass, and aren't much slower :-)

SimenB · 2020-04-07T22:36:56Z

On my machine npmt test seems a tiny bit faster, not sure how much of a benchmark that is, though. probably not at all 😀

Master:

  Time (mean ± σ):      6.358 s ±  0.548 s    [User: 7.007 s, System: 0.792 s]
  Range (min … max):    5.776 s …  7.328 s    10 runs

This branch

  Time (mean ± σ):      6.243 s ±  0.390 s    [User: 6.979 s, System: 0.751 s]
  Range (min … max):    5.929 s …  6.992 s    10 runs

ljharb · 2020-04-07T22:47:14Z

when all the optionals are done, we can compare the "total time" on the travis builds :-p

ljharb · 2020-04-08T00:01:37Z

Looks like travis took 3:34:08 (214.13 min) on master, and 2:33:30 (153.5 min) on the PR, across 176 jobs, so that's about 28% faster, or about 1min 13s vs 52s - seems like an improvement to me!

Appveyor needed a few reruns, so we'll see how that shakes out.

…symlinks

SimenB · 2020-04-08T13:04:10Z

@ljharb one thing - using the native realpath might cause issues, see nodejs/node#7175

From what I gather require uses the native realpath (although I haven't verified by looking at source code) so I still think it's safe (or even correct) to change, but a heads up seems warranted in any case

ljharb · 2020-04-08T15:37:16Z

It looks like perhaps using https://www.npmjs.com/package/fs.realpath would handle those issues, when using the native realpath?

…symlinks (#217)

- [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)

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

SimenB mentioned this pull request Apr 7, 2020

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

Closed

ljharb force-pushed the native-realpath branch from 23c54bb to 2982656 Compare April 7, 2020 22:26

ljharb approved these changes Apr 7, 2020

View reviewed changes

ljharb and others added 2 commits April 7, 2020 22:13

[Test] update appveyor config

dfda431

[patch] sync/async: use native realpath if available to unwrap …

535ec22

…symlinks

ljharb force-pushed the native-realpath branch 3 times, most recently from f8001b6 to 535ec22 Compare April 8, 2020 06:23

ljharb merged commit 535ec22 into browserify:master Apr 8, 2020

ljharb mentioned this pull request Apr 8, 2020

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

Merged

SimenB deleted the native-realpath branch April 8, 2020 07:24

ljharb pushed a commit that referenced this pull request Apr 15, 2020

[patch] sync/async: use native realpath if available to unwrap …

bab1166

…symlinks (#217)

ljharb added a commit that referenced this pull request Apr 15, 2020

v1.16.0

301c07d

- [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)

nicolo-ribaudo mentioned this pull request Apr 16, 2020

Don't include "resolve" in @babel/standalone babel/babel#11432

Merged

snyk-bot mentioned this pull request Apr 18, 2020

[Snyk] Upgrade resolve from 1.15.1 to 1.16.0 saiichihashimoto/babel-plugin-extension-resolver#204

Merged

pos777 mentioned this pull request May 21, 2022

resolve result is inconsistent with require.resolve if windows virtual drive is used #283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: use native `realpath` if available to unwrap symlinks#217

feat: use native `realpath` if available to unwrap symlinks#217
ljharb merged 2 commits intobrowserify:masterfrom
SimenB:native-realpath

SimenB commented Apr 7, 2020 •

edited

Loading

Uh oh!

SimenB commented Apr 7, 2020

Uh oh!

ljharb commented Apr 7, 2020

Uh oh!

ljharb left a comment

Uh oh!

SimenB commented Apr 7, 2020

Uh oh!

ljharb commented Apr 7, 2020

Uh oh!

ljharb commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

ljharb commented Apr 8, 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!

SimenB commented Apr 7, 2020

Uh oh!

ljharb commented Apr 7, 2020

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB commented Apr 7, 2020

Uh oh!

ljharb commented Apr 7, 2020

Uh oh!

ljharb commented Apr 8, 2020

Uh oh!

SimenB commented Apr 8, 2020

Uh oh!

ljharb commented Apr 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

SimenB commented Apr 7, 2020 •

edited

Loading