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

Support bare internal mappings #30

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

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
Copy link
Collaborator

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
Copy link
Owner

jkrems commented Mar 12, 2019

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
Copy link
Contributor

ljharb commented Mar 12, 2019

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.

}
}
```

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

We could allow arbitrary paths or not. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Owner

jkrems commented Mar 13, 2019

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
Copy link
Contributor Author

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
Copy link
Owner

jkrems commented Mar 13, 2019

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
Copy link
Contributor Author

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
Copy link
Owner

jkrems commented Mar 13, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor

ljharb commented Apr 4, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

ljharb commented Apr 4, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

ljharb commented Apr 4, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor

ljharb commented Apr 4, 2019

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
Copy link
Contributor Author

npm aliases don't offer the following features:

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
Copy link
Contributor

ljharb commented Apr 4, 2019

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
Copy link
Contributor Author

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
Copy link
Owner

jkrems commented Aug 14, 2019

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

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