tc39 / proposal-source-phase-imports

Proposal to enable importing modules at the source phase
https://tc39.es/proposal-source-phase-imports/
MIT License
129 stars 9 forks source link

Editorial: Partial revert of pr 36, only passing the specifier to the host #38

Closed nicolo-ribaudo closed 5 months ago

nicolo-ribaudo commented 1 year ago

This PR reverts the changes to HostLoadImportedModule (except for the newly added note about preloading) and FinishLoadingImportedModule. "Import phase modifiers" do not affect how a module is loaded (other than for potential unobservable and unspecified optimizations, such as speculative preloading), and as such it shouldn't require changes to the specified host interface.

The parameters of HostLoadImportedModule are now again:

There are the layering changes I was trying to propose in https://github.com/tc39/proposal-import-reflection/pull/36 (or better, it's how I was proposing to not do layering changes), and I would like to discuss if your (@guybedford) concerns regarding preloading are still solved with the text proposed in this PR, given that it still includes the note.

Given that preloading is unspecified, the spec doesn't need to expose spec-level facilities used by it. Implementations are free to do whatever magic they want and read all the internal state of ECMA-262 (think about how, for example, JITs use runtime type information to decide when and how to optimize a function), as long as they don't change the semantics defined by the spec. If an implementation really wanted it's internal API to match the signature of host hooks (this is not required anywhere) and thus do not read internal state of ECMA-262, they could still detect the phase using the _payload_ parameter: as long as doing so doesn't cause any observable[^1] change, it's not a violation of the "payload must be treated as opaque" requirement, similarly to how unobservably[^1] using the phase is not a violation of the requirement that it must not affect loading.

A possible middle ground is https://github.com/tc39/proposal-import-reflection/commit/4cb525d77a129f1a2430768d931f63dae19bbfaf (commit diff on top of this PR), which keeps the clear distinction of responsibility of the parameter while still passing the (in that case) explicitly useless/redundant phase to the host hook.

PR preview at https://nicolo-ribaudo.github.io/proposal-import-reflection.

[^1]: Obviously all of this is observable by a server that receives an HTTP request earlier than expected, but it's not observable using ECMA-262 facilities.

guybedford commented 1 year ago

I understand the reasoning and I can get behind the non-explicitness of preloading argument. I guess I still don't understand what the issue is with exposing the phase to the host hook. That is, if there are two ways to achieve the same thing, and one is simpler and more explicit, and another is longer and less explicit (ie this PR has a positive diff and introduces a new record), then it is up to the more complex approach to justify why that approach is necessary.

At the very least I'd be interested to hear more about the middle ground approach you mention in https://github.com/tc39/proposal-import-reflection/commit/4cb525d77a129f1a2430768d931f63dae19bbfaf. Are you saying that keeping the parameter out of FinishLoadingImportedModule is also important here?

littledan commented 1 year ago

This PR looks great. I'd like to see it land as part of moving this proposal to Stage 3--otherwise, I'd want to know what hosts are expected to do with the new information/flexibility.

littledan commented 1 year ago

I dunno, as long as we make sure that hosts never choose their behavior based on the phase, it's fine to not land this patch. I agree with Guy that it'd be good to get as close as possible to unifying the dynamic and static import paths, but that would probably take some much broader refactoring, so it shouldn't block Stage 3.

guybedford commented 1 year ago

I've updated the PR title to note this is primarily an editorial change at this point. Happy to continue discussion.

guybedford commented 5 months ago

I assume this is no longer needed.