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

Bug: ModuleEvaluation had breaking change to not return completion value #1227

Open
bmeck opened this Issue Jun 15, 2018 · 11 comments

Comments

Projects
None yet
4 participants
@bmeck
Member

bmeck commented Jun 15, 2018

Somehow missed this change a while back

This value is used by Node in order to create some reflection https://github.com/nodejs/node/blob/master/lib/internal/modules/esm/create_dynamic_module.js

@bmeck bmeck added the spec bug label Jun 15, 2018

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 15, 2018

Member

Can you explain the issue in more detail? Remember that the spec has a blanket clause allowing returning JS values which implicitly gives you a normal completion for that value.

Member

domenic commented Jun 15, 2018

Can you explain the issue in more detail? Remember that the spec has a blanket clause allowing returning JS values which implicitly gives you a normal completion for that value.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jun 15, 2018

Member

Can you link to the part of the spec in question?

Member

ljharb commented Jun 15, 2018

Can you link to the part of the spec in question?

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Jun 15, 2018

Member

@domenic does that apply to concrete methods? I thought that concrete methods needed to return the value the spec says they return.

https://www.ecma-international.org/ecma-262/6.0/index.html#sec-moduleevaluation states that it completes with result

newer spec https://tc39.github.io/ecma262/#sec-moduleevaluation completes with undefined

Member

bmeck commented Jun 15, 2018

@domenic does that apply to concrete methods? I thought that concrete methods needed to return the value the spec says they return.

https://www.ecma-international.org/ecma-262/6.0/index.html#sec-moduleevaluation states that it completes with result

newer spec https://tc39.github.io/ecma262/#sec-moduleevaluation completes with undefined

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 15, 2018

Member

Yes, it applies to everything. "Concrete methods" are just abstract operations that conform to some interface.

Member

domenic commented Jun 15, 2018

Yes, it applies to everything. "Concrete methods" are just abstract operations that conform to some interface.

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Jun 15, 2018

Member

@domenic it still looks like this changed from result to undefined.

Member

bmeck commented Jun 15, 2018

@domenic it still looks like this changed from result to undefined.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jun 15, 2018

Member

Right, that makes sense, since no caller uses that value.

Member

domenic commented Jun 15, 2018

Right, that makes sense, since no caller uses that value.

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Jun 15, 2018

Member

@domenic maybe, I'm not sure on this though since we do have some exposure of the result being used by a host platform. From a spec point of view this seems fine if we don't consider host usage, but I'm pointing particularly that a host is using this result.

Member

bmeck commented Jun 15, 2018

@domenic maybe, I'm not sure on this though since we do have some exposure of the result being used by a host platform. From a spec point of view this seems fine if we don't consider host usage, but I'm pointing particularly that a host is using this result.

@ljharb

This comment has been minimized.

Show comment
Hide comment
@ljharb

ljharb Jun 15, 2018

Member

This seems more like a layering issue than a spec bug then.

Member

ljharb commented Jun 15, 2018

This seems more like a layering issue than a spec bug then.

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jun 25, 2018

Member

If there are some hosts which concretely point to an abstract algorithm and say, this operation of mine calls that abstract operation and returns the result, it may be helpful for that host's documentation to have stability in the JS spec. This is sort of a layering issue, like for HTML calling into abstract operations.

I'm not sure what abstract operation you're talking about (I don't see one called ModuleEvaluation) and what host API is referring to that. I'm also not sure which spec change made a difference. It would be helpful if you could give these details.

Member

littledan commented Jun 25, 2018

If there are some hosts which concretely point to an abstract algorithm and say, this operation of mine calls that abstract operation and returns the result, it may be helpful for that host's documentation to have stability in the JS spec. This is sort of a layering issue, like for HTML calling into abstract operations.

I'm not sure what abstract operation you're talking about (I don't see one called ModuleEvaluation) and what host API is referring to that. I'm also not sure which spec change made a difference. It would be helpful if you could give these details.

@bmeck

This comment has been minimized.

Show comment
Hide comment
@bmeck

bmeck Jun 25, 2018

Member

@littledan it used to be called ModuleEvaluation now it is just Evaluate, I have links above. I chose to use the original name prior to some reorganization that went on since that had the behavior desired and is the name of the link to the section still.

Evaluate on Source Text Module Records is the concrete algorithm in question. This corresponds to the API in v8 for v8::Module::Evaluate

Member

bmeck commented Jun 25, 2018

@littledan it used to be called ModuleEvaluation now it is just Evaluate, I have links above. I chose to use the original name prior to some reorganization that went on since that had the behavior desired and is the name of the link to the section still.

Evaluate on Source Text Module Records is the concrete algorithm in question. This corresponds to the API in v8 for v8::Module::Evaluate

@littledan

This comment has been minimized.

Show comment
Hide comment
@littledan

littledan Jun 25, 2018

Member

Ah, sorry, missed that, thanks for the references!

Member

littledan commented Jun 25, 2018

Ah, sorry, missed that, thanks for the references!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment