Update spec to throw on export * from dynamic modules #11
Conversation
guybedford
force-pushed
the
reexport-throw
branch
2 times, most recently
from
f090e96 to
5fec4a1
Compare
|
The corresponding ECMA262 spec change for this is here - guybedford/ecma262#3. And the implementation has been updated at guybedford/v8#3. |
|
I've been following along and digging the progress made on the dynamic-modules proposal. However, this latest update, making |
|
I agree that Dynamic Modules are a way to smooth transition to native modules from transpiler output. However, I would like to avoid blocking this first iteration and would be much more appreciative of follow ons that unlock behavior as consensus is reached across committees rather than preventing any progress. It feels like some of the issues are due to concerns about virality of changes and I don't think that discussion on if the virality is acceptable if it affects purely ESM<->ESM linkage will be short. @jdalton would it be fine to move forward with it as it stands and use a follow on proposal that can discuss that specific syntax? As it stands ahead of time tools can convert |
I'd like to hold up and not move forward just yet. I'd rather we as implementors and spec writers take more of this issue head-on rather than push it to the dev-users and/or bundler/transpiler authors. To that end I've asked @zenparsing to investigate an alternative approach, building off parts of this proposal, that aims to give Node the freedom to more fully support named exports of CJS in ESM while also reducing added complexity in ECMA262. |
|
@jdalton is there any expected changes that differ from this spec? If there is, is there a timeline we can constrain ourselves to while diverging to encourage a time that we can expect to make a joint decision on how to approach moving forward? |
Yes, some likely.
I'll defer to @zenparsing for projected timelines. |
|
I'd like to continue down this path and not block this PR as we have multiple scenarios that allow us to make progress here:
Unless there are clear explanations of why a block needs to be done on this spec if a different spec is being suggested to get around the block I would find little reason to not land this PR. This PR allows this spec to move forward towards implementation, and we can treat a different competing spec as something else, either as a follow on or completely moving separately. If a block is desired, we can ask for quorum amongst @nodejs/modules but for now I don't have clear explanation on the competing spec idea, nor on the semantic differences being requested that mean it cannot be treated as a follow on. My hope is that we can land this as is. |
|
Throwing affects Node's transparent interop story in what I see as a negative way, so I'd like to hold off. As a developer, one of the wins of transparent interop for me is not having to concern myself with the parse goal of modules/packages I'm importing/re-exporting. If The holidays and end of year vacations are upon us. Things will likely continue tending towards slow-to-no response until the new year. As a Node Modules Working Group and TC39 member I'm not in favor a proposal with these kinds of traps. Let's pick this up in the new year with @zenparsing and others. |
|
I am fine waiting on a different proposal or follow on, but see no need to
wait weeks on this PR. Even if you see transparency as diminished this PR
reserves the proper space for you to provide that functionality separately.
We have had multiple talks about deadlines and this PR follows a similar
approach of starting minimally and iteration just like our kernel. I do not
see a need to wait until the end of the year or even further.
…On Mon, Dec 17, 2018, 3:29 PM John-David Dalton ***@***.*** wrote:
Throwing affects Node's transparent interop story in what I see as a
negative way, so I'd like to hold off. As a developer, one of the wins of
transparent interop for me is not having to concern myself with the parse
goal of modules/packages I'm importing/re-exporting. If export * from
throws for CJS modules I have to concern myself with the parse goal which,
to me, diminishes having transparent interop in the first place.
The holidays are upon us, and things will likely continue tending towards
slow-to-no response until the new year. As a Node Module Working Group and
TC39 member I'm not in favor a proposal with these kinds of traps. Let's
pick this up in the new year with @zenparsing
<https://github.com/zenparsing> and others.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#11 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOUo7H9qo3lSRTVFkD36Jo5uSvKZknkks5u6AzFgaJpZM4ZIvwj>
.
|
This specific PR is a problem. Without this PR there may be edge case complexities, but there's still syntax support which, to me, is a better place to be. We can continue to work on solutions to reduce complexities in the new year. |
I agree, and that it can and should be done separately. The new year is still more than 2 weeks away and we can add features if solutions are found to things discussed here. If you can elaborate what edge case complexities you are worried about it may enlighten us to why this PR which was a response to edge case problems and removing problematic cases while allowing future possibilities is not the same as what you are suggesting we do. If we are removing things/ blocking them due to edge cases like this PR is doing, it seems exactly what you are seeking to do. |
I get it. It can be a bummer, but it's that time of year.
My mention of "edge case complexities" was directly referencing the PR description:
I dig reducing complexities, but not at the expense of syntax support. |
Does that mean if the syntax cannot be supported due to problems with it, we should drop this proposal? These "complexities" are logical problems and not implementation specific as I understand it. Would lack of 100% compatibility be a problem, because at it stands these modules are already observably different from SourceTextModuleRecords at runtime. Whenever we are able to bring this to a meeting, we should lay out what compromises are acceptable. Absolute mandates of full transparency are not possible and so we must categorize and refine what we are willing to compromise on in order to ease overall developer usage. If we throw out the entire proposal because of an edge case not being able to work, we are doing a disservice compared to removing the edge case as this PR seeks to do. If we do wish to wait to discuss this, we should come back ready to talk about it and not review it repeatedly. If you have specific concerns about the compromises of this and how a different compromise is acceptable you can bring them up then; but, I would prefer they be concrete and actionable at that time to avoid reiterating and relitigating repeatedly as we think about deadlines. If there are no actionable alternative compromises that are proposes that can reach consensus, we should consider the minimal consensus approach even if it is not ideal. I want to make progress and aid developers to use as much syntax as possible even if we have to trim support for some smaller subset. A good example would be providing a show of overwhelming usage that cannot be migrated of
Notably, this would put ~70% of usage as always going through compilers already. If these packages otherwise would be going through pure ESM<->ESM when transitioning they also would not be affected. |
Support for
I'm not wanting to put the work on devs or transpilers here. I rather we work through the behind the scenes issues so they don't have to spend time on this. Making |
We can implement all sorts of things, we have a turing complete language, that doesn't mean it works correctly or is safe. This is unrelated to the issues being discussed. We could take this to hyperbolic levels and say that we can implement Babel instead of ESM, but we are not doing that in this group. Implementability is independent of this issue and not helpful when discussing things if we do not get an explanation of why the problems found when implementing this feature are not problematic. This issue is from experiences in implementation not theoretical. This PR is about implementation.
If they transition to ESM they won't have these issues, and if transpilers continue to support it devs won't face issues. That leaves support up to transpilers and never on devs, which are tools that always are going to need to continue supporting features as roll out. None of this seems actually impactful unless transpilers like Typescript refuse to support the host for implementation reasons, such as why they refused to add
This is unrelated to the logical problems being discussed. Please provide concrete alternatives or else I will assume this means that without support you are requesting we stop proposing support for importing CJS. The intent here is to exclude the syntax in a way that allows for continued efforts to support it if some champion such as yourself seeks to work with this proposal to make the syntax work properly. So far, there has been little discussion of how you seek to change this proposal except a reaction to leaving future compatibility as possible while logical problems being resolved as being unacceptable. I do not think any one is wishing to ban this syntax without reason, but we have reason to do so and a lack of explanation of how to avoid the problems discussed here. This lack of explanation leads me to have little to work with and discuss except a vague sense that you wish to stop any ability to import CJS if it doesn't work fully transparently for some undefined constraints. As such, I defer to my previous comments as well about needing to define what are acceptable compromises rather than just calling out what are not. Due to lack of explanation, lack of alternatives, and the future compatible nature of this PR. I still stand firm in stating that we should attempt to merge this as no problematic behavior caused by merging this is being shown by comments in this thread. |
I believe we can work through this without cutting syntax. The holidays and end of year vacations are upon us. I don't think we should push this change though at this time.
If everyone used ESM then transparent interop wouldn't be a concern. However, I don't see that as a likely outcome for some time.
A potential benefit of transparent interop is allowing devs to drop transpilation for ESM syntax. Delivering transparent interop that still requires transpilation is not ideal.
Possible ecosystem impact of a change is something to consider.
I think other avenues should be explored before we cut syntax. I've asked that folks hold-off making these changes during the holidays and end of year vacations so that others can better participate. |
|
added modules-agenda tag |
You have not explained how.
If they continue to use transpilers they also would not have a concern.
This is why I've gathered info on transpilation info, that is showing 70%+ of usage being behind a transpiler even if we ship full undetectable interop. That means 70% of usage in the wild would remain unaffected even if interop was detectable such as through inability to use some subset of syntax. We should tie our discussions to data and logical problems and I have not been seeing that in this comment thread when being asked to not move forward.
We can always revert this PR if a solution is found. I don't see holding off on this PR as preventing others from participating given that they can come and alter the decision at a later time. My request to land this is from the practical nature of our current state of the world, data on the usage backs up impact, the future compatible nature of unbanning the syntax, and the ability to revert this PR. If you have concerns about the ability to revert this PR and/or lack of ability to participate in reverting this PR we can address those, but they do not seem to relate to the logic issues in this PR and should be discussed in a meeting about process of iterating our implementation. We do not need to immediately land this PR but I would not like to have it remain open for months as we discuss every alternative when it could show an explicit problem that we are working around by merging this PR. |
|
I'd note that if we are to bring discussion to TC39 about this in January, the PR would need to be ironed out before January 18th. Otherwise, we will need to wait until March at which point I will be on parental leave. |
We haven't added support for either in TS for the same reasons, so maybe not the best example to bring up - host support is currently unstable and we're not in a position where we can officially support both a changing experimental version and the final version. We've tried that with decorators and it bit us but good. ❤️
Wasn't this issue prompted by concerns over the implementability of I like dynamic modules. I like everything it's allowing. I like it's direction. And, from reading some of the associated issues, this spec change is just meant to be a stepping stone - a stricter requirement than is necessary which can get relaxed in the future. For that reason I'm OK with it. Like JDD, I'd be disappointed AF if we shipped without this resolved (since from a static analysis perspective it's a pretty big change), but at the same time I'm OK with merging it if we can agree to move forward, work from this base, and work towards resolving this issue more fully.
There are alternatives to outright banning the construct. Guy mentions in his presentation one of them - only banning circular esm/cjs export starts, which are the underlying problem. Also, off the top of my head, allowing dynamic-ness to taint the containing esm module if a circular dynamic import causes it to be necessary - and in my mind, that's pretty reasonable. If you have an esm-only graph, it'll never happen, but if you introduce dynamic modules, then there's a chance they affect how your other modules resolve. Guy maybe talked about some of these during the TC39 meeting and they decided banning the construct was the least complex option for now? I wasn't present and can't see the exact notes (they aren't published yet AFAIK), so I don't know. To me, it would just seem to be a matter of justifying the complexity to the relevant parties (which, to keep interop clean, implementation complexity seems justified to me).
They will have a concern, because all that'll happen is that we'll be forced to detect the scenario (heuristically) and mark it as an error since there's nothing it can transpile to (when targeting esm). It's important to think of esm as both an input format and a target format. ❤️ |
I have had different accounts in personal discussions.
It was about fallout from actually implementing such. See some comments in the referenced PR at top of issue.
We need to weight these compromises. I and others are concerned over the viral taint of such a strategy involved above because it exactly means that heuristics need to be added to ESM<->ESM imports even if they are not directly involved in dynamic modules. It would mean hoisting the problem instead of just at point of the dynamic module being imported to the entire graph for transpilers and hosts to deal with. Releasing the ban would still mean that heuristic interop concerns must be persisted to transpilers; however, these heuristics would affect all Transpilers already do very complex heuristics IMO and this is just more for the continued reasons as the other heuristics. The lack of ability to remove other existing heuristics is not removed by this spec. That means that interop will remain complex for any transpiler. We should weigh the ability to stop other heuristics before stating that more heuristics are untenable. |
See #11 (comment) and #11 (comment). Because of holidays and end of year vacations (me included, I'm traveling across the country tomorrow) things won't likely be ready until after. Let's pick this up in the new year. |
|
@jdalton even with wanting to propose things after, the ability to revert this PR, adopt a new spec, and the future focused nature of how this PR allows follow ons. I see little reason to wait for after the new year on this. You can come back at the new year and show your alternatives and it won't be hindered by us landing this PR. |
|
AFAIK the TC39 breakout session did not mandate cutting syntax. Since there is no mandate I see no reason to push through a PR that does so over possible alternative at this time. Since there is folks against cutting syntax I think we should hold-off merging until others have the time to properly ingest the issue and are in a place to better participate. |
Ah... but allowing the dynamic module taint to spread means users don't have to think about it. The module loader is just doing whatever's needed to load the code in whatever format it is into whatever desired format. It does just defer the problem to runtime, but that's just what happens with circular imports in cjs today - you get a partially resolved exports object. And it's reasonable (to me) that a graph which includes cjs (or cjs-capable structures) can essentially devolve into a cjs graph in the worst case. |
There is work and even an implementation that @guybedford has done. This is not from solely TC39 discussions and we need to bring this feedback to TC39. In addition, TC39 members are aware of this spec, and have even commented on the PR referenced. We cannot use breakout sessions of TC39 as a gatekeeper to prevent movement forward. As I've stated, we can always revert this PR. I do not see a hinderance to participation after the holidays being done by merging it. If you have concerns about the ability to participate if this PR lands or the ability to revert this PR due to process we can discuss that. However, given that we can agree on some minimal subset of support and iterate on to add more support, landing this PR follows how we have reached consensus in other areas and have managed to make progress rather than stalling out in discussions of wholesale alternatives. We should continue to learn from our progress and follow directions that are giving us the ability to move forward by landing minimal agreeable states and iterating, not by refusing to reach an agreement on even minimal agreeable state. If the ban on We can work forwards towards supporting |
That's kind of counter to the PR description since it uses "feedback from the last TC39" breakout session as justification for the change.
I'd rather move forward with edge case complexities intact, ideating-on and putting-effort-into PRs and solutions that reduce complexities without cutting syntax, instead of opting to cut syntax as the first course of action. My sense is that transparent interop with |
|
I think what @jdalton may be getting at can be viewed in a more general sense regarding all our early implementation efforts: If we choose to throw when an optimal behaviour is not yet defined we restrict the ability for others to use these efforts cautiously to refine other problems or to try to address the actual problem. So imho, a stable implementation with a similar pitfall warrants throwing, otherwise we may be better off communicating those pitfalls and for the rest of us as relatively seasoned developers to respectively invest in appreciating the hurdles for what they really are. |
|
A stable implementation does not exist while it's under a flag. |
|
Yes, that's the point I was trying to make. Since it is not user-facing (behind a flag or similar) it should not be eagerly throwing while the undesired behaviour is being refined. If the undesired behaviour can be handled suboptimally for now and until it can be solved right, the suboptimal path does not block parallel efforts or at least makes it easier for more of us to appreciate the trade-offs to presented down the road. |
|
Since it's unstable, it can throw all the time, or at any time, for any reason. It's unstable - nothing about it can be, or should be, relied upon. We can delete it entirely in one patch version and add a different implementation in another. |
|
Absolutely, but forcing it to throw for |
|
Landing this PR is "working to get there"; there's no reason that all edge cases have to be resolved in the implementation prior to unflagging. Nobody will be impacted, because surely nobody is relying on an unstable implementation. |
|
I disagree, because if I or others from the modules team rely on non-cyclic |
|
I just want to clarify, I am not blocking this (interim). I am merely voicing concerns which I hope others can appreciate to allow everyone to bring their maybe relatively limited skills to good use and to try to work from a position where different perspectives may all present with valid points. |
Can you explain this? If you can use named exports instead what are we seeking here and how would banning |
|
@bmeck We can all agree that what people wanted all along is That said, today with all the different flagged implementations Writing code is not my concern, it is code you don't see coming that you absolutely cannot control and yet such code is only rarely ever going to I am not the best person to argue for seamless or otherwise importing of cjs in esm, but it is hard for me to not be concerned if a valid syntax in ESM throws the entire graph because this happens somewhere in my dependencies (hacking those is far more ugly than any recoverable side effect). |
|
@SMotaal this error would be early and nothing using it would pass its own test suite before being published. |
|
I think we are talking about two completely different things. But, as you said though, not PR related at this point 👍 |
|
Merged and as discussed in the last meeting we can continue to investigate the alternatives. |
The feedback from the last TC39 meeting was to make
export * from 'dynamic-module'throw so that we can avoid a lot of the edge case complexities of dynamic modules.This implements that on top of the modified ECMA262 patch at guybedford/ecma262#3.
Nice that it is mostly a negative diff as we no longer need to track all the namespaces which star export from a dynamic module.