Skip to content

Resolve builtins with --no-browser-field, fixes #1654 - #1826

Open
goto-bus-stop wants to merge 4 commits into
masterfrom
no-browser-field
Open

Resolve builtins with --no-browser-field, fixes #1654#1826
goto-bus-stop wants to merge 4 commits into
masterfrom
no-browser-field

Conversation

@goto-bus-stop

@goto-bus-stop goto-bus-stop commented Apr 4, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Currently when browserField: false or --no-browser-field is used, builtin modules like path and stream are not resolved correctly. They go through the resolve module, which simply returns the module name for core modules. Then module-deps tries to read the file named path which doesn't exist (usually).

With this patch, builtin modules are checked before calling into the resolve module.

TODO:

@shuse2 shuse2 mentioned this pull request Apr 15, 2019
Closed
@dbousque

dbousque commented Aug 25, 2019

Copy link
Copy Markdown

Thanks @goto-bus-stop for the fix. I can confirm that it works and fixes an issue I have. Is it planned to release this fix ?

@dbousque

dbousque commented Sep 25, 2019

Copy link
Copy Markdown

Thanks @goto-bus-stop for checking again. Is it ready to merge ?

@shuse2

shuse2 commented Mar 10, 2020

Copy link
Copy Markdown

We are having this problem too.
Would it be possible to merge this PR for the next release?

ljharb
ljharb reviewed Mar 10, 2020
View reviewed changes
Comment thread index.js
? function (id, opts, cb) {
if (!opts.basedir) opts.basedir = path.dirname(opts.filename)
// Resolve builtin modules.
if (self._mdeps.options.modules[id]) return process.nextTick(function () {

@ljharb ljharb Mar 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should use resolve’s isCore functionality, i think?

@kevinswiber kevinswiber Apr 13, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ljharb I don't believe so, as custom built-ins can be fed into Browserify, and those won't be picked up by resolve.isCore. See: https://github.com/browserify/browserify/blob/master/index.js#L551-L553

@ljharb ljharb Apr 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If those can override the default ones, then that's a fair point - but then browserify should be using resolve's node-version-specific logic for the defaults, which it doesn't appear to be.

@kevinswiber kevinswiber Apr 14, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@ljharb Yes, it overrides the defaults, in a way. It's just a direct replacement of built-in resolution. Browserify doesn't use the node-version-specific logic, and that's a separate issue from this one that's worth logging. I'm maintaining a fork right now with this change, and I'd like to be able to depend on the officially published version. Is there anything else holding this up or can it move forward?

@kevinswiber

kevinswiber commented Apr 13, 2020 •
edited
Loading

Copy link
Copy Markdown

Let me know if there's anything I can do to help push a fix for this over the finish line.

Use case: I'm using Browserify to custom-tailor third-party modules for use within the Postman Sandbox execution environment (see: https://explore.postman.com/templates/7170/browserify-cdn-modules).

Being an Electron app that hosts an even more restricted execution environment, a Browserify CLI call needs to look something like this:

➜ browserify \
--require @stoplight/spectral \
--no-browser-field \
--insert-global-vars __filename,__dirname,process,global \
--exclude assert,buffer,events,fs,os,path,punycode,querystring,stream,string_decoder,timers,url,util

Because of this bug, I get the following error:

Error: ENOENT: no such file or directory, lstat '/Users/kevin.swiber/src/postman-browserify/assert'

This PR fixes the issue for me.

Thanks!

@ryanio

This comment was marked as spam.

Sign in to view
@mikixing

This comment was marked as spam.

Sign in to view
This was referenced May 31, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
+1 more reviewer
@kevinswiber kevinswiber kevinswiber left review comments
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@goto-bus-stop @dbousque @shuse2 @kevinswiber @ryanio @mikixing @ljharb