-
Notifications
You must be signed in to change notification settings - Fork 29.8k
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
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
Proposal: Support more import assertion types #40766
Comments
IIRC in the browser there's also discussions about having two different kinds of WASM imports, one for the |
Assertions are explicitly meant to be optional in the TC39 spec; it would be a mistake for node to decide to adopt constraints that it doesn't have. |
For folks who are not familiar with the context, the reason the HTML spec requires assertions for JSON is because file extension are meaningless for a web browser (they rely on MIME type, which may or may not be in phase with the file extension), and to let users import safely third party JSON modules (e.g. if The reason assertionless WASM Module/Instance may still be on the table is because IIRC the debate hasn't settle whether WASM modules should be able to import JS modules. If WASM module can import JS, having an assertion is not useful because it would have the same "level of privileges" as JS modules. If not, the assertion would be mandatory for the same reasons as JSON modules. (I wasn't part of any of those discussions, if you think I'm not representing the current state of the debate accurately, please correct me). |
@aduh95 data imports are already supported and are affected by MIME. |
And certainly HTTPS imports will be, of course, and #36328 is in progress. I think once that lands, Node will have many of the same concerns as browsers, and will need the type assertions for the same reasons. I also think it would be very confusing if an assertion is required for some imports but not others based on whether the module is loaded from the filesystem versus over the network. |
I think it would be much more confusing if an application that uses no network loading was forced to pay a tax solely because of the existence of those features. If they would be, then if anything that’s an argument against adding those features in the first place. |
Overall consumer compatibility to run in multiple environments is definitely a bigger concern for me than avoiding having an assertion in my code. If we don't support import assertions for example, we won't be able to write code that runs in both. Node should be working to make code work in a way that aids our users and I don't see explicitly trying to avoid compatibility as a useful thing. Tooling can be run ahead of time in both cases to mitigate w/e UX people want, but having the end result be the one that is maximally compatible is ideal. |
If we take the use case of a JS module written as part of an npm package, the author can't know what protocol their code would run from: it could be local FS (from a |
@aduh95 indeed; importers aren't supposed to have to know the format of the module they're importing - requiring an assertion imposes this burden. |
That's a valid point, for something like |
@aduh95 since the specifier explicitly specifies the filesystem, it would only matter if a loader wanted to load it over the network instead, and also wanted to require the assertion. |
Assertions mix an integrity concern into code which is otherwise not concerned with integrity. IMO, assertions really should be fully optional in node (and honestly, reduce application author freedom), and you really should be able to specify them alongside your dependency hashes in whatever app-level manifest you're using to lock down package integrity, rather than inline. The application author should have the freedom to override dependency resolutions without undue burden. These assertions reduce the freedom application authors have today for what I understand to be no tangible upside in node. So sure, node should understand the syntax and types and be capable of reading it and validating it so it can handle more code intended for a browser, but I don't think it should be encouraged or required - browsers have invented this inline integrity check because they don't have application (or page-level) dependency manifests in use anywhere, and are trying to solve the problem without one. Node already has one in common use, though. I'm really not a fan of this whole "node needs to clone the WHATWG HTML spec behavior and nothing else" direction. If I wanted to use headless chrome, I would. |
What this does bring up though: we may need a |
While the idea of a module being naively able to be replaced is sound in theory there are problems in practice that require an ESM wrapper for a facade in general practice:
So while the underlying implementation of the algorithms provided by modules may be replaced by other more optimized forms, for all the relevant formats currently you still want a JS wrapper around it so the assertion type would be stable. |
This was discussed in depth in #39921, including a TSC meeting regarding the topic. Basically, making assertions optional introduces significant complexity on Node’s own loading infrastructure regardless of browsers. It was decided by the TSC that import assertions could ship as required for now, with later consideration around making them optional.
I think #40766 (comment) covers well why I don’t think there’s any reduction of freedom involved here. In terms of the tangible upside, it’s clear: Node could unflag JSON and potentially Wasm module support. Since browsers have already shipped support for
This is already possible via loaders, and one of the tests in #40250 was a loader that does just that (enabling assertionless
There’s already an open PR for HTTPS imports, and regardless of whether that lands sooner or later I don’t think we want to make design decisions that would preclude the potential support of that feature. Once HTTPS imports are part of Node, we will have many of the same concerns and limitations as browsers; and for compatibility we would generally want to follow the same solutions as them. Or put another way, the least risky approach is to adhere to both the TC39 and HTML specs whenever possible, as then we’ll have confidence that we won’t run into compatibility issues later. Nonstandard behavior could be opted into via loaders, flags, |
Node isn’t constrained by what browsers do - why would we want it to be? |
@ljharb these are not constraints imposed by browsers, there is complexity from purely the node side. Actual initial surface API seems sane, but as @GeoffreyBooth points out in the previous comment, these require more UX investigation due to added implementation complexity and forwards API design spaces. E.G.
I'd suggest all claiming things about this being constraints imposed by browsers please try and see the actual problems that are not being solved by such claims since a variety of these are completely unrelated to anything from web specifications. Things like Things like claims that not needing to know the format of the module allowing optimized module routing simply are real in all forwards facing things that I know of for C++, WASM, or even JSON and generally will want/need an ESM wrapper. Things like claiming there is no upside to preventing code that will fail when it gets served in a way that is compatible with browsers is actively making an assertion that code in node wouldn't be ported to be served in a situation that wants to match (such as loading a module in both a browser and in node). This isn't related to adherence to browsers, but instead is a simple avoidance of a problem for developers. So, before we claim "things will be possible if we have ambiguous data" we should be clear that this is not something about browsers for most of these concerns. I do still think and did argue at TC39 to ensure import assertions should be allowed to be optional, but I don't see lots in the comments about the actual implications of doing so except from people on the loaders team currently. I'm becoming convinced I don't want them to be optional based upon various reasons, none of which are from wanting to follow WHATWG. |
Precisely - because a runtime check would be wholly extraneous when you control the entire application and can guarantee consistency prior to execution (because you installed and validated everything in advance). So sure, you should have the choice to assert consistency at runtime, but the common scenario in node doesn't require it. Like, if you've already checked that your
Sure. But nice defaults go a long way. And it's pretty much a no-go for library code, since "oh, by the way, my llibrary only works with loader X on" is usually a nonstarter. I don't think I've ever seen a
Again, sure, but far and away most code is and will be file based. Data URLs and blobs are esoteric at best, and I, personally, have yet to be convinced that
Technically the loader already has a kind of conflict resolution, since a json document can be loaded by both the cjs and esm loader - any conflict resolution within the esm loader should probably be consistent with that behavior (which is to say: the same underlying instance appears in all possible caches). If implementation complexity was actually a huge concern, I don't think we'd adopt esm at all, tbh - the complexity it has been adding is a massive PITA, for not just node core but the ecosystem as a whole; what's one more cache concern within node on top of that. Side note: json import assertions are kind of a PITA that get in the way already - they make it impossible to, eg, swap between a dynamic js file on
I mean, |
Yes, doing this is good because things like module system exploits are fairly common as an attack vector. I don't think "paranoia" is a good way to attempt to attack the argument for using runtime checks.
There is disagreement on "nice". The goals of the node project technical priorities do include web compatibility and that is part of the disagreement of the trade offs for what is the best thing to do here.
I think you are once again somewhat attacking the efforts of other here and trying to frame it that people's work is silly or not relevant in order to state it has no bearing of concern in this discussion. If the problem is the ability to use code w/o requiring CLI top level declaration of loaders due to developer friction that is much different from coming out and stating like you did that Loaders aren't relevant for broad use.
Once again, this is trying to state that a potential future is not relevant to be wary of for designing Node. I disagree with this tactic of argumentation as it is simply trying to preclude any future development in directions that have some desire from the ecosystem/community at large. Just because a feature is not going to be used by an individual does not make that feature something that should be allowed to be completely discarded in discussion of potential problems/trade offs being taken here and now.
I've argued that they shouldn't share the same cache as CJS can invalidate assumptions about JSON files when they do share a cache. So, even this argument that they should share the same cache seems to be in disagreement.
This seems unrelated to the issue at hand and appears to be an attempt to treat a separate discussion as a sliding slope for allowances in this discussion. It would be good in this case to understand without the claim of ESM being complex how the concerns of import assertions themselves are not a valid concern with added burden.
Even with WASM interface types (which are not yet finished or standardized) they are not aiming for 100% coverage of all possible conversions.
I'm unclear on how this is aided by allowing ambiguity in the loaded dependency. It would be good to understand what practical "freedom" this is allowing. |
I'm mildly sympathetic, but it seems a bit like they could just use a JS file in production. So the problem is easily fixed without adding implementation complexity to loading of modules. This wouldn't work though if the dynamic JS file uses named exports for example. So care needs to be taken to stay on that very small safe overlap of default exports. |
Again, the point is choice - some developers may be paranoid and want to reiterate integrity checks at runtime, sure (just like how some defer typechecking until runtime), but yet others may be running node on an embedded system where every cycle of overhead costs silicon budget - developers should have the power over what node does wherever possible. Robbing developers of that choice reduces node's flexibility and reduces its suitability for use in certain projects.
Yep - and web compatibility doesn't imply web-apis-only. (I mean, that's clearly a nonstarter, right?) So long as node understands web-bound code in the same way web runtimes do, allowing more flexibility than what web runtimes offer should be in the wheelhouse of acceptability.
This is a practical observation of the world as it exists today - cjs loaders exist, and some do see use, but there're not a library-to-library thing, and even if there are plans to change that, I think hinging the designs of other major systems on those plans looks a lot like wishful thinking - minimally, how the ecosystem will react to future "nice esm loaders" is entirely unknown. Like plugins, "You can write a plugin for that" is very often a polite way to say "No, we won't do that", without actually engaging with the underlying design questions, which is why I don't think they're a great thing to point to as a potential solution.
It's not an argument about a future, but rather a current reality. Think too far ahead and you just overdesign things, IMO.
🤷♀️ I don't care what exactly they do when multiple loads occur, I'm just saying a choice has already been made, somewhere, so just being consistent with that choice is what matters. The point is just that a behavior is already implied - the implementation within the esm loader simply doesn't exist yet (it only exists across loaders). If you wanna change the behavior then go for it, it just implied intra-loader conflicts should probably be handled the same way.
Oh, it is, it's definitely just a gripe that found it's way in (though you really have to admit the esm implementation in node is complex compared to the cjs implementation - and only having the cjs one (or only the esm one, though that's impractical) would definitely be simpler), but to continue if you enjoy the offtopic content, feel free to expandI've started to have to field questions on a daily basis about export maps now that we're trying to get people to adopt them, and wow, it is not pretty. I'm definitely feeling like I contributed to a major failure here, since export maps were supposed to be a solution and not a problem in and of themselves, and it's gotten me _really_ on edge about esm as a whole in node. I'm finding that anyone who wasn't involved in developing esm for node has trouble understanding how esm in node works without some detailed and nuanced instruction. I knew it'd be an issue, but I didn't think it'd be _this bad_.
Sure, but they cover a lot of them - and a lot of node packages are a single function that takes some bools/numbers/strings in and spits a bool/number/string out - the area covered is quite large. Sure, complex stuff with object references and streams or something may require complex marshalling and wrapping, but the point is that it isn't always required, and that a developer shouldn't need to wrap it (and may not) if they don't need to do any of that.
If
Imagine, for a moment, that that's not practical because the parser chokes on it because it's too large (meanwhile the json version is fine because the json parser is much more efficient). Or a some other concern, like the static data in prod coming from a file that's written by some legacy system (while the dynamic js file generates dummy data for testing). It's hypotheticals all the way down, but the point is just that node's kinda getting in the way when it doesn't feel like it needs to - you can set this structure up with the tools we already have, but it's very difficult to make the code actually run in both environments (there's probably some way to make it work, but I doubt it'd be as pretty as an import that just gets swapped out). |
Re the swapping-in-production theoretical, that’s similar to an example I wrote for the docs in #40785 for when const assertType = process.env.NODE_ENV === 'development' ? 'javascript' : 'json';
const data = await import('dep/data', { assert: { type: assertType } }); |
This issue was moved to a discussion.
You can continue the conversation there. Go to discussion →
While working on #40250, the PR that added support for the import assertions syntax in general and required it for JSON imports, @aduh95 and I discussed a follow-up PR that added support for other formats. There are two in particular that we think should be added:
webassembly
: Animport
of a WebAssembly module would need the assertion just like JSON modules do:import './file.wasm' assert { type: 'webassembly' }
. This follows the examples in the import assertions proposal. (WebAssembly imports are currently flagged behind--experimental-wasm-modules
and I think this flag should remain, at least until it becomes clear how browsers will handle WebAssembly imports.)javascript
: Per the HTML spec, animport
statement with no type assertion impliesassert { type: 'javascript' }
. The explicit version should also be supported, though, because there are cases likeimport(url, { assert: { type: dynamicType } })
where it is useful for Node to accept an explicit assertion type for modules that don’t necessarily need one.The ES module loader has five formats defined internally (
builtin
,commonjs
,module
,json
,wasm
), each of which is mapped to either an assertion type (currently only'json'
, for thejson
format) or the “implicit type,” which currently means that an import assertion type isn’t allowed for that format. I propose that we mapwasm
to'webassembly'
; and that we mapbuiltin
,commonjs
andmodule
to'javascript'
. The'javascript'
type could either be explicitly specified (import './file.js' assert { type: 'javascript' }
) or implied (import './file.js'
).These changes will align Node with the HTML spec, which is more detailed than the TC39 import assertions proposal. While Node of course doesn’t need to follow both the TC39 and HTML specs, it would be risky to not do so; any ways in which we deviate from the HTML spec could lead to cases where users can’t write code that can run without modification in both platforms. One potential example is if we disallow
type: 'webassembly'
and browsers require it, or vice versa. At least for the time being, until it becomes clear how import assertions shake out across the ecosystem, it seems prudent for Node’s default behavior to adhere to the most restrictive spec and for us to wait to relax our requirements until it’s clearly safe to do so. (I’m referring to the Node’s default ESM loader; user loaders can modify Node as they wish and violate the spec as they please.)Another more-strict part of the HTML spec is that the assertion type is part of the module cache key. The TC39 spec proposal allows different ways to handle multiple imports of the same module with different assertions; the HTML spec team settled on this solution, and I think that Node should align with the HTML spec in order to have consistent behavior across platforms. What this means in practice is that assertion types cannot be optional, in the sense that an import without a declared type is treated as if it had included
assert { type: 'javascript' }
; no other type can be implied. I wrote more about this in a suggestion for the import assertions proposal.This also positions Node to support new module types in the future. The only other type that I’m aware of any browsers supporting is
'css'
, which I haven’t read discussion around Node supporting, but currently there is no way to import native (.node
) modules in ESM. This could be remedied by following the example of JSON and Wasm, by creating a new loader format and assertion type for such files. cc @MylesBorinsI’m opening this issue so that this thread can be the place for discussion of this proposal, so that we can limit the chatter on the PR that implements this to technical review of the code in that PR; “product” or “feature” debate should please happen here. cc @nodejs/modules @nodejs/loaders
The text was updated successfully, but these errors were encountered: