Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: resolve main reference to other modules #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pemrouz
Copy link

@pemrouz pemrouz commented Sep 10, 2016

Related: #83

Notes:

  • I tried to keep changes to a minimal

  • I reused this check to differentiate references to module/file.

    This does mean that there are a number modules that specify "browser": "browser.js" which would be invalid. It seems better to try align this with how require works for less confusion in the long-term (i.e. require('./browser.js') vs require('browser.js'), but if there is a lot of breakage an additional check could always be added for .js. I'm happy to go on a PR-spree for the browserify modules that do this.

  • The tests added pin down behaviour for the following few use cases:

    • browserify . on a module which has browser set to another module
    • browserify main.js (where main.js is referenced by main in package.json)
    • require('module-a') from module-b (note: the replacement for module-a is resolved relative to itself)

@@ -0,0 +1,4 @@
{
"main": "./main.js",
"browser": "module-b"
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the feature that previously did not work?

Copy link
Author

Choose a reason for hiding this comment

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

yep

On 10 Sep 2016, at 17:28, Roman Shtylman [email protected] wrote:

In test/fixtures/node_modules/module-t/package.json:

@@ -0,0 +1,4 @@
+{

  • "main": "./main.js",
  • "browser": "module-b"
    this is the feature that previously did not work?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

@defunctzombie
Copy link
Collaborator

@substack @sokra @Rich-Harris for additional review

@pemrouz
Copy link
Author

pemrouz commented Dec 6, 2016

bumping.. is there anything else required before we can land this?

@pemrouz
Copy link
Author

pemrouz commented Jun 16, 2017

@defunctzombie ..?

@dy
Copy link

dy commented Nov 28, 2018

@defunctzombie @sokra @Rich-Harris ping

@goto-bus-stop
Copy link
Member

This does mean that there are a number modules that specify "browser": "browser.js" which would be invalid.

i don't have numbers on this but i've definitely seen it in the wild so support for that should not be dropped.

it looks like "browser": "barename" did not previously resolve to ./barename.js so that change should be safe.

@goto-bus-stop
Copy link
Member

@pemrouz if you're still interested in working on this i can review later this week. no guarantees that defunctzombie will have time in the near future to do merges/releases ofc :)

i don't think webpack uses browser-resolve these days, unsure about rollup. it'd be good to check if webpack's enhanced-resolve already supports this too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants