module: unflag import assertions #39921
Conversation
aduh95
added
the
semver-minor
nodejs-github-bot
added
c++
|
We should be sure to have a test that has both w/ and without |
|
@bmeck any chance you would be able to write the edge case test you mentioned in the other PR and send a commit to my branch? I'm not sure I understand exactly what would be the expected behavior, so tests would be very helpful. |
aduh95
force-pushed
the
unflag-import-assertions
branch
from
1f85c01 to
38e330b
Compare
This comment has been minimized.
This comment has been minimized.
aduh95
force-pushed
the
unflag-import-assertions
branch
from
239f3ed to
8754a41
Compare
|
@bmeck I've implemented in ece7a7d...733deb8 the suggested cache implementation you mentioned in #37375 (comment), PTAL. I've used a |
It fails in both cases, or succeed in both cases when loaded with |
|
@aduh95 my concern is that it shouldn’t be possible to import json with the assertion unless it’s possible to load it without the assertion. Sounds like this PR doesn’t create that scenario. |
|
@GeoffreyBooth I removed the assertionless syntax from the docs, as you requested. Removing it from the tests would not be possible as I explained above, we can discuss adding a |
|
In my mind, the two blocks are about two separate things:
Personally I think the code that would go under So I think the path forward is to remove the support for two forms from this PR, leaving only the assertion form, and then this PR can land; and then #37375 can land and we unflag JSON modules. If @aduh95 or others still want to propose the additional support for assertionless JSON imports, that could be opened as a new PR to add that and it can be debated on its own merits. |
Note that this is a spec recommendation (https://tc39.es/proposal-import-assertions/#sec-hostresolveimportedmodule):
Removing the possibility of this happening is worth discussing, but I think that's out of the scope of this PR. |
|
Neither JSON modules nor import assertions should be unflagged until you can import JSON without an assertion, since that scenario being possible is the only reason EITHER FEATURE EXISTS. When this was discussed in TC39 we were under the impression Node.js had reached consensus about this plan (that node would allow JSON modules to be imported both with and without the assertion, while browsers would mandate the assertion). This was communicated to the committee by the proposal champions and various node representatives. It is frustrating to see this consensus has been lost, after years of effort with TC39 and browser implementations. If this block persists, I think we will need to rethink how Node.js and TC39 interacts because of how important it is to build trust and good faith between these bodies in order to advance proposals. |
|
@ljharb those talks which I was present for and participated in were to my recollection about allowing the assertion to be omitted, not enforcing that a host such as node must implement it in a way that it is omitted. You seem to imply that discussion was done in bad faith which is not what I see or have participated in. If you want to use TC39 as a lever to force Node to implement something I am a bit worried. The different opinions of hosts and standards evolve over time for one point, and another is that the Node collaborators were not universally in those discussions and framing it as if all the consensus was only done outside of the typical way that consensus is formed for Node features is concerning. |
|
@bmeck the discussions were done with the explicitly stated assumption that node would allow the assertion to be omitted (of course, not that the spec would mandate anyone omit it, only permit it). I do not believe any of this was done in bad faith, including the block here. It seems that what happened is that I, at least, did not realize the potential for a collaborator to block at the last minute. I only provided consensus for advancement of both JSON Modules as well as Import Assertions, at any stage, because I believed node would allow omitting the assertion. Had I realized this block was a possible outcome, I would have not allowed either proposal to advance without at least a TSC decision beforehand to omit the assertion. Separately, I am highly concerned about a block on the philosophical grounds of "node must not have a feature that a browser lacks", and I think that would be a very worrying precedent for node. It's clear I've allowed my frustration and fear about this topic to drive me to make very strong statements in this thread, and I apologize that at times that means I've overstepped, or too-strongly characterized history or the opinions of others, and additionally that I've led some to believe I'm implying bad faith in the past or now - which was never my intention. |
|
@ljharb even with discussions about wanting to allow the assertion-less form, concerns about non-philosophical things are popping up as I'm trying to figure out the right way to let user provided loaders handle the races that occur if both forms are supported. Such a thing likely wouldn't have shown up until we looked at how to integrate with Loaders and after stage 3 was achieved anyway. |
|
@bmeck your technical concerns are valid and do not give me the same level of concern. I believe they can be overcome, and if they can't be, then that is a different and concrete sort of issue than a philosophical concern. |
| const finalFormat = finalFormatCache.get(job); | ||
| if ( | ||
| import_assertions.type == null || | ||
| (import_assertions.type === 'json' && finalFormat === 'json') |
There was a problem hiding this comment.
shouldn't this always check if the finalFormat is 'json' and not skip it if it is nullish?
There was a problem hiding this comment.
If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.
shouldn't this always check if the finalFormat is 'json'
we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.
There was a problem hiding this comment.
If import_assertions.type is nullish (I.e. assertionless import) and the previous job succeeded, we return the cached module. Because there was no assertion made, there’s no reason to check what format the module was loaded with. If/when we want to force assertion for some format, we would need to add some additional logic, but that’s not what this PR aims to do.
shouldn't this always check if the finalFormat is 'json'
we still want non-JSON modules to load (such as module, commonjs, etc.), I’m not sure I understand what you mean by that.
There was a problem hiding this comment.
Oh I think I get what you mean, indeed because json is the only valid value for an assertion type for now, indeed json is the only format acceptable here. I think we should keep the code ready for more values to be added, so when new assertion types are introduced, it won’t have to refactor the whole implementation.
There was a problem hiding this comment.
the problem is that we have 2 cache entries coalescing to the same module job and that makes me trying to figure out how a userland loader could recreate that behavior very complicated and I am definitely not confident in that collision being safe even with the mitigation I mentioned and requested above. currently we always have 1-1 fully resolved cache key to module instance. The concern is the many-1 model that we introduced by coalescing.
There was a problem hiding this comment.
it was pointed out to me that i misunderstood the state of this PR. I'm still unsure on how viable allowing both with and without assertion at the same time given my last review. I think having an actual meeting rather than PR comments would help clear things up as these discussions have dragged on long enough that my brain is having trouble understanding the actual state of the PR or potential eventual state of the PR
|
Based on TSC conversation today the TSC felt comfortable with separating landing this PR only supporting the form with the assertion and doing a follow up discussion to determine the viability of shipping the version without the assertion afterwards. This would require changes to the current implementation The thought here was that, at least in my impression, there would not be a world where we would ship ONLY the assertionless approach and not support the assertion approach as this would be explicitly breaking code that will work in the browser. |
|
I have a question for folks who participated in the TC39 discussions about this proposal: is it fair to assume that JSON modules will not be the only type of module that will be permitted to be imported both with and without an assertion? My thinking when working on this PR was that other module formats would be added to the list (maybe WASM, JavaScript even), and that JSON modules just happen to be the first. |
|
Wasm thread on import assertion requirement seems as yet undecided here - WebAssembly/esm-integration#42. I'm glad caution is being taken and consideration is being given to platform support, but given this PR doesn't ship anything unflagged I'm glad the TSC came to the conclusion it seems good to ship. |
|
@aduh95 the intention was for the burden of the assertion to only be required when it was necessary, which for filesystem JSON modules, it’s not. The same would be true of wasm modules, presumably, since they have a file extension, so I’d expect those assertions to be optional as well. |
As long as the assertionless form is optional for any future formats, then we won’t need to support two syntaxes for the same format. Assertionless could be only for JavaScript and assertion-required could be every non-JavaScript format. Node wouldn’t need to support two syntaxes for the same format, for any format. If the assertion becomes allowed for JavaScript, then we would need to support two forms. Is there a proposal for that? It’s much simpler from a maintenance burden perspective and from a loaders development DX perspective to only support one syntax per file type. I don’t want to add “assertion or assertionless for the same file type” support unless and until we’re required to. Edit: I suppose another option that the standards bodies could take would be to add new formats as assertionless-only, similar to JavaScript (like maybe that’s how Wasm goes). And then we’d be in the same position, where JavaScript and Wasm are assertionless and JSON (and possibly other formats like CSS or HTML) are assertion-required; and if/when the day comes that any format can be more than one syntax is the day that we add support for two syntaxes for the same format. |
Would it make sense to discuss that in a @nodejs/loaders meeting? I could join as a guest if that's OK. Or we could schedule a call outside of the usual WG meetings, I've never done that myself so I suppose we'd need a volunteer to set that up for us. |
I added the I’m not sure we need a meeting just yet? Per #39921 (comment) I think if @aduh95 or someone else is willing to open a PR that simply lands the assertion syntax for JSON modules without the additional support of making the assertion syntax optional, such a PR can land and we can unflag JSON modules. Then either this PR or #40210 would only consist of making the assertion syntax optional, which would merit a meeting for sure. But I’m not sure we need discussion around that before we’ve landed the other two PRs first? |
mhdawson
removed
the
tsc-agenda
|
Converting to draft to avoid confusion with #40250, which is the version of this being actively developed. |
|
Closing as #40250 is the version under active development. |
Import assertion support has landed behind a flag in V8 for quite some time, this PR enables it by default in Node.js. This PR allows folks to write JS code that uses import assertions syntax. Only
type: "json"is supported for now, although JSON modules support in Node.js is still experimental and behind--experimental-json-modulesCLI flag.Refs: #37375 (comment)
Refs: https://tc39.es/proposal-import-assertions