-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
modules: runtime deprecate subpath folder mappings #35747
Conversation
Review requested:
|
2d4f98e
to
6e21041
Compare
6e21041
to
f258111
Compare
Codecov Report
@@ Coverage Diff @@
## master #35747 +/- ##
==========================================
- Coverage 96.41% 87.91% -8.50%
==========================================
Files 223 477 +254
Lines 73685 113133 +39448
Branches 0 24642 +24642
==========================================
+ Hits 71043 99462 +28419
- Misses 2642 7963 +5321
- Partials 0 5708 +5708
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@addaleax This is a runtime deprecation on two experimental features (subpath exports and subpath imports), so maybe it's not semver-major? |
@Trott Yeah, no strong opinion, it felt like it’s better to err on the safe side here to me, that’s all :) |
Co-authored-by: Jordan Harband <[email protected]>
Thinking some more about the backporting I wonder if it might actually be useful to backport this to 14. The warnings will only show for local package usage since we have good filtering to not show for third-party packages without I would still suggest holding out until 16/17 for any throw-by-default behaviour though, but it could be beneficial to backport the warning actually. I'm updating the PR description, but if anyone has any concerns let me know. For all the modules experimental status process and changes, this is probably one of the only few removals we've done actually! |
@addaleax given the above, would you be ok with removing the semver major label here? |
@guybedford As I said, no strong opinion :) Just the fact that runtime deprecations are semver-major by default, but if everybody else is good with it then I am too |
Ah I was not aware of that. Ok I'm completely fine with leaving the backport then. |
@guybedford Is there a specific question? |
@addaleax yes, would you be ok with removing the "semver-major" label from this issue? |
@guybedford I still am, yes |
Landed in c9acb9e |
PR-URL: #35747 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Myles Borins <[email protected]>
PR-URL: #35747 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Is there any discussion, somewhere, on the pros and cons of leading node.js away from the import maps specification? |
@dcleao that doesn't seem directly related to this issue. However, you may be more interest in policies which have a more node tailored schema and you can generate from import maps (though import maps can't quite do everything policies do). @giltayar was stating he was going to try and write a converter ( https://github.com/giltayar/import-map-to-policy it seems ). Per if node is moving away from import maps, I'd say we are moving closer to them not further way. Exports are quite a different feature so that might be what is causing confusion. |
@bmeck How can it not be related to this issue? The connection is pretty clear to me. The subpath folder change applies to both imports and exports. Exports are quite a similar feature to imports, mostly symmetrical in nature and design and which were afaik derived from the ideas of import maps. You can read more about the other side of the coin here: WICG/import-maps#232 and WICG/import-maps#244. |
@dcleao the thing that was deprecated here is not actually a feature import maps supports, because this one also exposes the entire directory, and every filecontained within (including extension/index lookup for CJS). |
I am sorry, I must be mistaken then. Is this issue not related with the deprecation of https://nodejs.org/api/packages.html#packages_subpath_folder_mappings ? |
@dcleao they were not designed by import maps, that might be the confusion. |
@bmeck They weren't designed to be identical to import maps, but I'm almost sure they were inspired, motivated by, triggered by, and, most importantly, designed to work with import maps. Even if you insist that this was not the case, given the causal and proximal appearance, their syntactic and semantic similarity, and related talks by maintainers of both sides on both repositories, I'm sure this was how most people understood the relation of Node's imports and exports to Import Maps. At a minimum, I'd expect being able to compile Node's imports and exports to Import Maps, for the subset of features which are expected to work and be useful on Node and the Browser. This included being able to convert a simple export folder mapping to an Import Map's, same syntax and semantics, import mapping. This being deprecated, in favor of pattern mappings, there's no longer a way to convert one to the other... It's broken, for the normal use case. I'm still trying to digest how you can say these two technologies are unrelated... |
You still can do that compilation. this feature was designed as an escape hatch from the encapsulation and explicit requirements of the exports field. Using it makes it harder to generate an import map (although it was of course possible). |
I mean... policies predate import maps, but exports is after import maps; I do think the features are dealing with similar data and problem spaces but not trying to solve the same problems. The similarity though doesn't mean they are actually intertwined. Design choices like import maps having ambient authority to import vs policies having limited authority by default for example causes divergence. Choices around scoping on package boundaries for "exports" vs import maps being a flat namespace is another.
Unfortunately import maps are not tailored for various use cases so other features outside of them are being created to serve these purposes. This lack of features from import maps makes having a complete compatibility unlikely. I'd agree with @ljharb that compilation to import maps from other features is likely the approach to take. |
In fact, given the following folder pattern mapping: "./foo/*": "./foo/*.js" it can be converted to one or more "regular" folder mappings, if all of the Yet, if there are 200 JS files below
Unfortunately, I did not fully understand what you meant. What is "this feature" referring to? The "folder mappings" being deprecated? Or, the new "folder pattern mappings"? |
In the sense that they become forced to evolve together — no, surely not. But would the ecosystem gain from "imports"/"exports" and "Import Maps" evolving together? Yes, I think it would.
Policies deal with additional concerns compared to Import Maps.
The concept of packages does not exist as a first class citizen in the "web", nor it does in Import Maps. Package boundaries are somewhat erased when represented as Import Maps, but the same name mappings apply. Or, could you be referring to whether it is allowed to use parent relative paths (
In essence, both of the features, "imports" and "exports", have/had a corresponding equivalent representation in "Import Maps", apart from some minor differences. There are some features of "imports" and "exports" that do not have a correspondence in Import Maps:
It is only "Policies" which handles other quite distinct responsibilities. So what other features or use cases are you referring to that justify these not evolving hand in hand? I think that this discussion reveals that there could be value in a document clarifying the expected path of NodeJS's "imports" / "exports" features and the differences it has to "Import Maps". |
Browsers have different constraints and affordances than node does, and as such, while it's useful for a number of reasons to maximize overlap, it wouldn't actually make sense to try to make either one exactly match the other. "exports" governs both CJS and ESM, for one, and browsers don't have CJS. |
I'm actually unclear on this. As shown beneath this statement the feature sets aren't the same so evolving them together would be around interoperability not around having the exact same data scheme. What does "evolving together" mean? Move all the work for node into WHATWG (browser) standards / not implement things unless they are in browsers? For more clarity, removing folder mappings like in this PR would bring "exports" closer to what import maps provides since it removes a feature that import maps don't support. "scopes" in import maps need to have a single destination for their specifier mappings.
I don't understand this. Perhaps clarification on how they are the same as import maps would help. "imports" is pretty close, but "exports" is kind of the inverse of import maps to my perspective. I would agree that we don't have a need to deviate more than is necessary and don't really want to cause friction but I'm unclear on how we could be aiming to be more compatible except by removing features that may be intrinsic to things like how "exports" works.
I'm actually referring to specific cases where the behavior of "import"/"export" vary based upon non-compatible semantics from import maps. Things like conditions that you mention, fallbacks, etc. are pretty easy to understand but can't be exactly emulated by import maps. However, things like preventing resolution ("cascade" in policy terms) at package boundaries for "import" and allowing generalized exposing of subsets of directories are possible to do in import maps if you compile it out, but are quite verbose/hard to deal without some sort of tooling.
I'm unclear on "evolving hand in hand" here, WHATWG is a browser spec place that isn't exactly a place that Node can participate very well in. See various efforts by Guy Bedford ( https://github.com/guybedford/import-maps-extensions ) to try and expand the spec to be more feature complete with the tings we are talking about. However, matching the browser constraints and not implementing useful features for node is unlikely, and the same for browsers removing the demanded constraints for their environment. It would be helpful to understand your suggestion that they evolve hand in hand. Like @ljharb I don't see being able to only accept features that the web allows as a valid path forward. Ensuring that you can generate import maps from Node's features seems somewhat feasible, but like stated above some features won't work so it would be on best effort.
I think such a thing would be great if someone wants to write it. I don't think it should be done so in an adversarial way though so biasing towards one or the other being "the right way" would be non-productive I think. They are just slightly different in purpose. |
Unfortunately, we're not being able to converge. Although somewhat repeating myself, I will try to summarize my point of view as clearly as I can.
I'm looking forward for extensions to the current WICG import maps with concepts which apply to all environments, as well as with concepts which apply only to some environments. It seems reasonable to try to use the WICG import maps as the trunk on which at least some of these extensions will lie. For concepts which don't have an exact equivalent on some of the environments, I'd still expect that we'd strive for making useful conversions possible, and practically feasible. I hope that this clarifies the expectations that I have for the future of NodeJS's packages and modules, and its relation to the Web's WICG import map and its future, related extensions. Maybe it's just me, maybe I'm just confused, but, also, maybe, I'm the only guy speaking out. |
This creates an explicit runtime deprecation warning when using the folder mappings in
"exports"
or"imports"
.The implicit documentation deprecation was already made when the exports patterns PR was landed in #34718 and fully replaced all documentation references to this.
The warning is carefully scoped to not apply to external packages in node_modules, unless explicitly using the
--pending-deprecation
flag.Tests are included for local and external, require and import, imports and exports and self-resolution cases.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes