FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Support custom names for "browser" field resolution by feross · Pull Request #1918 · browserify/browserify · GitHub

Support custom names for "browser" field resolution - #1918

Merged
feross merged 1 commit into
masterfrom
custom-browser-field
Aug 9, 2019
Merged

feross merged 1 commit into
masterfrom
custom-browser-field

Conversation

feross commented Aug 8, 2019

Copy link
Copy Markdown
Member

Fixes: #1917

Comment thread index.js
resolve(id, opts, cb)
}
: bresolve
: typeof browserField === 'string'

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

are we sure nobody is putting a string value now, and relying on it being a noop?

What if it's an empty string?

feross Aug 8, 2019
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right now any value other than false (including an empty string) just does normal "browser" field resolution.

We've never supported any value other than false for this argument and since this is an additive change, I'd argue it could be released in a minor version. But I'm also fine with releasing it in a major version to be extra safe, though I think it's a bit overkill.

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry, that was two questions :-) you answered the first, but not the second - after this PR, should an empty string be allowed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The simplest rule would be: if the type is string, then we look for that key in package.json and apply the "browser" field spec.

So, then an empty string would look for an empty string key in package.json.

But it's also likely to be a bug, so also I'm okay with throwing an exception in this case.

feross force-pushed the custom-browser-field branch from 2af69b6 to 8213b64 Compare August 8, 2019 23:07

feross commented Aug 9, 2019

Copy link
Copy Markdown
Member Author

@substack took a look at this and said it looks good on IRC. I'm going to merge it now.

feross merged commit 4c04949 into master Aug 9, 2019
feross deleted the custom-browser-field branch August 9, 2019 05:25

feross commented Aug 9, 2019

Copy link
Copy Markdown
Member Author

16.5.0

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Support custom names for "browser" field resolution

2 participants


Back | FazBrowse Home | New Git URL