Support bare internal mappings #30

guybedford · 2019-03-12T15:29:23Z

This PR includes an example and description of bare internal mappings as an extension to the proposal.

This has been discussed before, and opens up opportunities for extending the behaviours to internal requires allowing full control over both internal and external resolutions.

GeoffreyBooth · 2019-03-12T15:47:58Z

Two thoughts, not necessarily deal breakers:

How would this interact with the require algorithm? Supercede it?
How would this work for deep imports? We would check the package.json for every deep imported file? (I guess we're doing that anyway.)

jkrems · 2019-03-12T16:05:34Z

One question: A common pattern in universal packages is to remap an implementation detail using the browser field in package.json. Would the expectation be that we'd encourage them to use a bare specifier for these? Or will we extend the relative path mappings to also apply to $packageBaseURL/$path?

ljharb · 2019-03-12T16:29:02Z

I think in general, every path should be resolved/normalized at every opportunity, so that you can always use any path anywhere, and they map as one might intuitively expect.

ljharb · 2019-03-12T16:29:58Z

README.md

  }
 }
 ```

 Within the `"exports"` object, the key string after the `'.'` is concatenated on the end of the name field, e.g. `import utc from '@momentjs/moment/timezones/utc'` is formed from `'@momentjs/moment'` + `'/timezones/utc'`. Note that this is string manipulation, not a file path: `"./timezones/utc"` is allowed, but just `"timezones/utc"` is not. The `.` is a placeholder representing the package name. The main entrypoint is therefore the dot string, `".": "./src/moment.mjs"`.

+For keys not beginning in `.`, these are mappings which apply internally to any imports made from within the package itself. So `src/moment.mjs` could contain for example `import dep from 'localdep'` which would load from a package-relative location. For example one could define `"moment": "./src/moment.mjs"` allowing the package to refer to itself by name as well in this way. These mappings cannot be used at all from outside of the package.


This seems to preclude being able to remap a file to a node_module. Also what about paths beginning in /?

Because this mapping operation happens early, it could even be a node_modules package lookup actually.

We could allow arbitrary paths or not. Thoughts?

I actually think this should forbid paths that don't start with ..

npm 6.9 has an alias feature that can be used to remap bare imports - I don't think there's any benefit in us duplicating it.

guybedford · 2019-03-12T21:15:09Z

@GeoffreyBooth

How would this interact with the require algorithm? Supercede it?

Yes, it happens as an early step before running the rest of the algorithm normally.

How would this work for deep imports? We would check the package.json for every deep imported file? (I guess we're doing that anyway.)

I'm not sure I follow this question? These apply to imports from within the package to bare names.

@jkrems can you clarify your question? I'm not sure I follow.

GeoffreyBooth · 2019-03-13T02:01:25Z

I’m not sure I follow this question? These apply to imports from within the package to bare names.

If my project has import 'some-package/some-file.js' and inside some-file.js it has import 'foo', you would need to look up some-package/package.json to find the mapping to foo. Which is fine, just pointing out that we need to look up the package.json relative to the file for this to work, rather than the one in the root project. In other words this builds on the package scope concept we’re using for "type".

jkrems · 2019-03-13T03:20:44Z

can you clarify your question? I'm not sure I follow.

I think a high level concern is that these entries have different "hoisting" rules. We're hoisting import map entries that start with ./ to the parent scope but keep entries that don't at the package scope.

/project-root
  /package.json - { dependencies: "x" }
/deps
  /x
    /package.json - { exports: /* see example this PR modifies */  }
  /localdep
    /package.json

If we'd generate an import map (ignoring protocol etc. and pretending that this is all at the root), it might look like this:

{
  "scopes": {
    "/project-root": {
        "x": "/deps/x/src/moment.mjs",
        "x/": "/deps/x/src/util/",
        "x/timezones/": "/deps/x/data/timezones/",
        "x/timezones/utc": "/deps/x/data/timezones/utc/index.mjs"
    },
    "/deps/x": {
      "localdep": "/deps/x/third-party/localdep.mjs"
    }
  }
}

To actually come back to the question above ("will we extend the relative path mappings to also apply to $packageBaseURL/$path"), expressed as an import map, it would be the following:

{
  "scopes": {
    "/project-root": {
        "x": "/deps/x/src/moment.mjs",
        "x/": "/deps/x/src/util/",
        "x/timezones/": "/deps/x/data/timezones/",
        "x/timezones/utc": "/deps/x/data/timezones/utc/index.mjs"
    },
    "/deps/x": {
      "localdep": "/deps/x/third-party/localdep.mjs",
      "/deps/x/": "/deps/x/src/util/",
      "/deps/x/timezones/": "/deps/x/data/timezones/",
      "/deps/x/timezones/utc": "/deps/x/data/timezones/utc/index.mjs"
    }
  }
}

I guess I answered by own question, this would likely become super confusing, especially with path remappings.

guybedford · 2019-03-13T14:10:22Z

Which is fine, just pointing out that we need to look up the package.json relative to the file for this to work, rather than the one in the root project

Yes exactly this works since we now have a concept of package scope.

I think a high level concern is that these entries have different "hoisting" rules. We're hoisting import map entries that start with ./ to the parent scope but keep entries that don't at the package scope.

I'm not sure I'd consider it as hoisting, but rather just more as thinking in terms of resolution rules for "bare package entry scope resolution" and "internal package scope resolution".

Agree the generated import map would like your example though.

To actually come back to the question above ("will we extend the relative path mappings to also apply to $packageBaseURL/$path"), expressed as an import map, it would be the following:

Thanks that example clearly indicates the question now. Yes I don't think we should do this - the relative mappings should apply only for the bare specifier entry into the package.

jkrems · 2019-03-13T16:20:18Z

Trying to apply this to a library that I maintain: https://github.com/groupon/gofer/blob/66dd001669091b9c1ba74e9dc08e08000761e80b/package.json#L8-L9. This is switching out a (non-public) implementation detail based on platform support. This proposal would suggest I'd do this for the standard module system:

{
  "exports": {
    "fetch-binding": ["./lib/fetch.browser.js", "./lib/fetch.js"]
  }
}

With the assumption that I can guard the first entry somehow (e.g. #29). It feels a bit weird to use a bare specifier to "abstract away" an internal override but it may be okay..?

guybedford · 2019-03-13T16:25:37Z

Right, so how do we guard the fetch.browser.js? Could we even do ["browser:./lib/fetch.browser.js", "./lib/fetch.js"]?

Note that one of the benefits of the indirection through a bare specifier is exactly ensuring a well-defined non-recursive resolver, as if we had relative paths become LHS entries in the map we would be introducing recursion :)

jkrems · 2019-03-13T16:34:59Z

I think people will (rightfully?) complain about the string-based meta data. But I might also just be biased by NIH and lean towards the earlier [{ "browser": true, "from": "./lib/fetch.browser.js" }, "./lib/fetch.js"] strawman. But I think that no matter what we found multiple possible syntax solutions there so I think it's a problem of agreeing on one, not finding a workable solutions. I could live with "pseudo-protocols" even though they may interfere with real protocols.

guybedford · 2019-03-13T17:16:47Z

My hope with conditionals is that we can not get too caught up in trying to craft the perfect generalization of environment detection (which is also a very difficult optimization IMO), but can rather focus on solving the use cases of having different entry points between modules browsers, legacy browsers, modules Node, legacy Node, and production and development environments can be done which I still see primarily as a combinatorial problem of the "main" / "module" / "browser" mains problem applied to arbitrary entry points.

guybedford · 2019-04-04T20:12:29Z

Here is an example of this feature in real-world use in the aws-sdk package:

https://github.com/aws/aws-sdk-js/blob/master/package.json#L51

{
  "browser": {
    "lib/aws.js": "./lib/browser.js",
    "fs": false,
    "./global.js": "./browser.js",
    "./lib/node_loader.js": "./lib/browser_loader.js"
  },
  "browserify": {
    "transform": "./dist-tools/transform.js"
  },
  "react-native": {
    "fs": "./lib/empty.js",
    "./lib/node_loader.js": "./lib/react-native-loader.js",
    "./lib/browser_loader.js": "./lib/react-native-loader.js",
    "./lib/core.js": "./dist/aws-sdk-core-react-native.js",
    "xml2js": "./dist/xml2js.js"
  }
}

In the above plain mappings are used to:

Map fs requires to an empty module in electron and the browser
Map require('xml2js') to a local provided version in react native.

Note how the environments being checked here are very simply - browser, react-native, electron.

Basically, this gives direct package map support to packages, to control their own mappings, which is just as important as allowing control of the exported entry mappings it seems to me.

Any thoughts further on merging this?

ljharb · 2019-04-04T20:16:37Z

browser is a defacto standard; having react-native follow that pattern seems like a good idea on their end.

Should perhaps, instead of exports, the key be named something environment-specific, if it's going to follow the lead of browser? node, perhaps?

guybedford · 2019-04-04T20:24:02Z

Please can we discuss naming and conditions in a separate issue.

This PR is specifically about the case of supporting plain mappings like "fs" alongside the internal mappings like "./lib/node_loader.js" for export maps, following in the browser field behaviour.

ljharb · 2019-04-04T20:26:35Z

So given that npm supports this already (https://github.com/jkrems/proposal-pkg-exports/pull/30/files#r264895480) why should node itself add support for it?

guybedford · 2019-04-04T20:33:54Z

npm aliases provide global aliases that apply only to top-level dependencies. This feature provides aliases which are scoped only to the current package and remain portable with that package such that they still work when installed in node_modules.

ljharb · 2019-04-04T20:38:24Z

I'm confused; I'm pretty sure npm aliases are per-package. It certainly only applies to top-level deps, but due to hoisting, you can add a transitive dep and it'll be the one used for your package.

guybedford · 2019-04-04T20:52:59Z

Ahh, right, I see they are.

The point is more that if we support array maps like import maps support and want to support the alias use cases then this extends that to those, as well as any other features these maps support as well for object forms.

ljharb · 2019-04-04T20:55:59Z

It seems more prudent to only allow non-bare paths here (ie, starts with . or /), so as not to overlap with the solution that npm already provides.

guybedford · 2019-04-04T21:00:08Z

npm aliases don't offer the following features:

Mapping to a local file
Mapping to a subpath file of a package
Supporting array fallbacks if this proposal supports it (Supporting browser stdlib fallbacks #31)
Supporting environment mapping forms if we support it (Alternatives and Environment Matching #29)

These all align with the use cases demonstrated in the example above.

npm aliasing is a different type of use case, which is different from the example above. I'm well aware of aliasing too - jspm implemented this feature back in 2013.

ljharb · 2019-04-04T21:02:33Z

For the first, "whatever": "file://./a/local/file".

Mapping subpaths is a fair point, but you can map to a local folder that has all the subpaths you want as actual folders. Fallbacks don't make much sense for node because everything's loaded off the file system.

There's already environment mappings via browser; that's not something node needs to solve.

guybedford · 2019-04-04T21:13:08Z

For the first, "whatever": "file://./a/local/file".

This doesn't work unless file is an actual folder to link I believe.

Fallbacks don't make much sense for node because everything's loaded off the file system.

We're thinking about the far-future here were import maps fallbacks as a feature allow environment handling. Consider environments with native support for wasi:... etc. where the fallback can allow a form of polyfill detection.

I can certainly appreciate if you think we should come back around on this one later though, but I'm just trying to ensure this spec captures as much of the features as the browser field as possible, which have been proven to be useful.

jkrems · 2019-08-14T21:29:56Z

I assume this is outdated and was replaced by #40 - feel free to reopen if that isn't correct!

Support bare internal mappings

d705ed9

ljharb reviewed Mar 12, 2019

View reviewed changes

guybedford mentioned this pull request Mar 13, 2019

Supporting browser stdlib fallbacks #31

Closed

jkrems closed this Aug 14, 2019

guybedford mentioned this pull request Jun 8, 2020

module: Private internal exports subpaths nodejs/node#33780

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support bare internal mappings #30

Support bare internal mappings #30

guybedford commented Mar 12, 2019

GeoffreyBooth commented Mar 12, 2019

jkrems commented Mar 12, 2019

ljharb commented Mar 12, 2019

ljharb Mar 12, 2019

guybedford Mar 12, 2019

ljharb Mar 12, 2019

guybedford commented Mar 12, 2019

GeoffreyBooth commented Mar 13, 2019

jkrems commented Mar 13, 2019 •

edited

Loading

guybedford commented Mar 13, 2019

jkrems commented Mar 13, 2019 •

edited

Loading

guybedford commented Mar 13, 2019

jkrems commented Mar 13, 2019

guybedford commented Mar 13, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

jkrems commented Aug 14, 2019

Support bare internal mappings #30

Support bare internal mappings #30

Conversation

guybedford commented Mar 12, 2019

GeoffreyBooth commented Mar 12, 2019

jkrems commented Mar 12, 2019

ljharb commented Mar 12, 2019

ljharb Mar 12, 2019

Choose a reason for hiding this comment

guybedford Mar 12, 2019

Choose a reason for hiding this comment

ljharb Mar 12, 2019

Choose a reason for hiding this comment

guybedford commented Mar 12, 2019

GeoffreyBooth commented Mar 13, 2019

jkrems commented Mar 13, 2019 • edited Loading

guybedford commented Mar 13, 2019

jkrems commented Mar 13, 2019 • edited Loading

guybedford commented Mar 13, 2019

jkrems commented Mar 13, 2019

guybedford commented Mar 13, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

ljharb commented Apr 4, 2019

guybedford commented Apr 4, 2019

jkrems commented Aug 14, 2019

jkrems commented Mar 13, 2019 •

edited

Loading

jkrems commented Mar 13, 2019 •

edited

Loading