Add missing ToString for assertion value in dynamic import() Runtime Semantics

[dandclark]

The Keys and Values in a ModuleRequest's [[Assertions]] list are supposed to be Strings. But, in the Runtime Semantics for the version of dynamic import() that takes an assertions argument, the value obtained from assertionsObj for a given assertion key inside the loop might not necessarily be a string.

This change adds the needed ToString() conversion before using the value in the assertions list.

[xtuc]

Enforcing a type is a good change, but i'm wondering what we should do for nested objects since that would technically allow them?

[littledan]

I think it's good to start with keeping dynamic import on the same level as static import as far as what it exposes. If we have hosts which want to take more complex objects as import assertion parameters, we should work out a syntax that works for static import as well.

[xtuc]

@littledan I was thinking about the consistency between static and dynamic import. If a host relies on nested object we should make it work the same in both cases, but currently with this change it only would work in the dynamic case

[littledan]

@xtuc The static case is covered by not having any syntax for a nested object.

[xtuc]

yes, my understanding is that dynamic import will possibility call ToString on a nested object, whereas static import would fail to parse.

[littledan]

Oh, I see. I guess it makes sense to simply check the type, rather than calling ToString, for future-proofing.

[xtuc]

Lifting restriction is easier than adding them later, I would suggest to make dynamic and static imports consistent and we can change the syntax over time to allow more types. How does that sound?

[littledan]

That sounds good to me; I just didn't understand what you were suggesting at first.

[xtuc]

No worries, i was being unclear.

[dandclark]

@xtuc @littledan Thanks, I agree that it makes more sense to keep this consistent with the static syntax by restricting to only strings. I've updated the PR to do that.

I also moved the is an entry of supportedAssertions check inside the loop. Otherwise, if there's something like this:

import("./foo", { assert: { maybeSupportedAssertion: { isAString: false } });

This should probably fail regardless of whether the host supports maybeSupportedAssertion or not, so the supported assertions check needs to be after we check the type of the value.

[xtuc]

Thanks for the update

[dandclark]

Just noticed I never landed this; doing so now...