[New] add `readPackage` and `readPackageSync` by SimenB · Pull Request #236 · browserify/resolve

SimenB · 2021-01-30T10:56:12Z

While readFile and readFileSync allows consumers to cache the FS operation, resolve will always do JSON.parse on the file content. readPkg and readPkgSync allows users to also cache that, if they want, avoiding the extra and potentially redundant JSON parsing.

This change largely makes readFile and readFileSync redundant IMO (as the only time resolve actually reads files is to read and parse package.json files), so for v2 it's a possibility to remove them.

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>

SimenB · 2021-02-09T09:30:04Z

@ljharb thoughts on this one?

ljharb

Thanks, this seems very straightforward!

I'll try to get this landed, and backported into a v1 minor, sometime today or tomorrow.

SimenB · 2021-02-09T14:50:06Z

Awesome, thanks @ljharb!

SimenB · 2021-02-09T15:29:53Z

Might be a good idea to land as readPackage instead? Not sure why I went with an abbreviation...

ljharb · 2021-02-09T21:44:56Z

ha, true, i'll buy some vowels

ljharb · 2021-02-09T22:06:14Z

@SimenB what do you think about making readFile/readPackage mutually exclusive? like, is there a use case where we'd want to accept both, given how you've built them to compose?

SimenB · 2021-02-10T12:44:51Z

@SimenB what do you think about making readFile/readPackage mutually exclusive? like, is there a use case where we'd want to accept both, given how you've built them to compose?

Definitely only need one of them. As mentioned in the OP I think readFile can (and probably should) be removed for v2, so making them mutually exclusive now certainly pushes people in that direction in a non-breaking way

lencioni · 2021-02-10T16:09:11Z

lib/sync.js

@@ -133,11 +142,7 @@ module.exports = function resolveSync(x, options) {
            return loadpkg(path.dirname(dir));


@SimenB Separately from this PR, it would also be nice if there was a way to short-circuit this recursive check for package.json file in every directory.

agreed, like the findClosestPackageFile I mentioned in jestjs/jest#11034 (comment)? All FS calls can be cached, tho, so in theory it's just a bunch of Map.get or something, so shouldn't be too bad

I'm open to a PR that enables that, for sure.

SimenB · 2021-02-10T19:02:38Z

lib/async.js

                    if (err) cb(err);
-                    try { var pkg = JSON.parse(body); } catch (jsonErr) {}
+
+                    var pkg = pkgParam;


if it wasn't obvious - this is done to satisfy an eslint rule about parameter reassignment (which packageFilter does below)

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

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>

- [New] add `readPackage` and `readPackageSync` (#236) - [Deps] update `is-core-module` - [meta] do not publish github action workflow files - [meta] create SECURITY.md - [meta] do not fail when `aud` is running and deps are not present - [meta] fix indentation in lib/core.json - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `array.prototype.map`, `aud`, `tape` - [Tests] skip `npm ls` check on older nodes

ljharb · 2021-02-11T14:47:02Z

Released in v2.0.0-next.3 and v1.20.0.

v1.20.0 - [New] add `readPackage` and `readPackageSync` (#236) - [Deps] update `is-core-module` - [meta] do not publish github action workflow files - [meta] create SECURITY.md - [meta] do not fail when `aud` is running and deps are not present - [meta] fix indentation in lib/core.json - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `array.prototype.map`, `aud`, `tape` - [Tests] skip `npm ls` check on older nodes

lencioni · 2021-02-11T14:51:26Z

Thanks!

Changes since v2.0.0-next.3: - [Breaking] ignore `SyntaxError`s from `readPackage`/`readPackageSync`, eg due to a malformed package.json Including all changes in v1.20.0 - v1.22.1: v1.22.1 - [Fix] support windows virtual drive paths (#284) - [Deps] update `is-core-module` - [meta] use `npmignore` to autogenerate an npmignore file - [meta] do not publish `appveyor.yml` - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `tape` - [Test] add tests for `"main": false` - [Tests] fix tests on node v12.0-12.2 - [Test] add some `sync` coverage - [Test] fix incorrect `require.resolve` paths logic; enable these tests - [Tests] avoid tests breaking on node 11.11 - 11.13 v1.22.0 - [New] add default support for `paths` to include `$HOME/.node_{modules,libraries}` (#273) - [Deps] update `is-core-module` v1.21.1 - [Fix] `bin/resolve`: allow `npx resolve` usage - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `aud`, `tape` - [Tests] add tests comparing `resolve.sync` to `require.resolve` v1.21.0 - [New] add top-level granular entry points - [New] add simple CLI util (#94) - [Refactor] `sync`: Do not throw on missing files in `isFile`/`isDirectory` (#256) - [Deps] update `is-core-module`, `path-parse` - [readme] pull in changes from default branch - [readme] remove defunct travis badge; update badges - [meta] backport FUNDING.yml - [meta] skip deleted files in `eclint` check - [meta] use `prepublishOnly`, for npm 7+ - [actions] reuse common workflows - [actions] pull in workflows from default branch - [actions] use `node/install` instead of `node/run`; use `codecov` action - [Tests] backport appveyor.yml - [Tests] add coverage for a malformed package.json - [Tests] only run `eclint` on intended files - [Tests] add coverage for absolute paths - [Tests] `invalid_main` fixture had an invalid "name" field - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `array.prototype.map`, `safe-publish-latest`, `tape` - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `aud`, `tape` v1.20.0 - [New] add `readPackage` and `readPackageSync` (#236) - [Deps] update `is-core-module` - [meta] do not publish github action workflow files - [meta] create SECURITY.md - [meta] do not fail when `aud` is running and deps are not present - [meta] fix indentation in lib/core.json - [Dev Deps] update `eslint`, `@ljharb/eslint-config`, `array.prototype.map`, `aud`, `tape` - [Tests] skip `npm ls` check on older nodes

[New] add readPackage and readPackageSync

b8d7def

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>

SimenB mentioned this pull request Jan 30, 2021

[discussion] Improving Jest performance with a custom resolver jestjs/jest#11034

Closed

SimenB force-pushed the read-pkg branch from e7127a1 to 98b6d92 Compare January 30, 2021 13:42

ljharb approved these changes Feb 9, 2021

View reviewed changes

ljharb force-pushed the read-pkg branch from 98b6d92 to 658aebe Compare February 9, 2021 21:56

lencioni reviewed Feb 10, 2021

View reviewed changes

SimenB commented Feb 10, 2021

View reviewed changes

ljharb force-pushed the read-pkg branch from 658aebe to b8d7def Compare February 11, 2021 00:00

ljharb changed the title ~~feat: add readPkg and readPkgSync~~ [New] add readPackage and readPackageSync Feb 11, 2021

ljharb merged commit b8d7def into browserify:master Feb 11, 2021

ljharb added a commit that referenced this pull request Feb 11, 2021

[New] add readPackage and readPackageSync (#236)

77eba10

Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com> Co-authored-by: Jordan Harband <ljharb@gmail.com>

SimenB deleted the read-pkg branch February 11, 2021 07:09

SimenB mentioned this pull request Feb 11, 2021

fix(resolve): add readPackage(Sync) DefinitelyTyped/DefinitelyTyped#51182

Merged

2 tasks

This was referenced Mar 15, 2021

Bump resolve from 1.15.0 to 1.20.0 in /client/app frogsbreath-games/eucre#223

Closed

build(deps): bump resolve from 1.17.0 to 1.20.0 ChatterlyOSE/Chatterly#591

Open

yarn(deps): bump resolve from 1.19.0 to 1.20.0 iaebots/iae#55

Merged

		@@ -133,11 +142,7 @@ module.exports = function resolveSync(x, options) {
		return loadpkg(path.dirname(dir));

Uh oh!

Conversation

SimenB commented Jan 30, 2021

Uh oh!

SimenB commented Feb 9, 2021

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB commented Feb 9, 2021

Uh oh!

SimenB commented Feb 9, 2021

Uh oh!

ljharb commented Feb 9, 2021

Uh oh!

ljharb commented Feb 9, 2021

Uh oh!

SimenB commented Feb 10, 2021

Uh oh!

lencioni Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

SimenB Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

ljharb Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

SimenB Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

ljharb commented Feb 11, 2021

Uh oh!

lencioni commented Feb 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

SimenB Feb 10, 2021 •

edited

Loading