tc39 / proposal-import-attributes

Proposal for syntax to import ES modules with assertions
https://tc39.es/proposal-import-attributes/
Apache License 2.0
599 stars 26 forks source link

Should `undefined` allowed in second assignment of import call? #52

Closed JLHwung closed 4 years ago

JLHwung commented 4 years ago

Currently the following use case

const assets = [
  ["ad-banner.mjs"],
  ["i18n/zh.json", { with: { type: "json" } }]
];

for (const [path, attrs] of assets) {
  await import(path, attrs);
}

will throw runtime type error as attrs can be undefined.

Should we allow import(moduleName, undefined) delegated to import(moduleName)?

ljharb commented 4 years ago

That seems like it'd allow a bug; why not the following?

const assets = [
  ["ad-banner.mjs"],
  ["i18n/zh.json", { type: "json" }]
];

for (const [path, attrs] of assets) {
  await attrs ? import(path, attrs) : import(path);
}
devsnek commented 4 years ago

by convention we treat undefined and no argument the same now, it should be allowed.

JLHwung commented 4 years ago

@ljharb The attrs can be an empty string but I am not gonna nit-picking.

Since the second argument is optional, developers may align it mentally to function calls where passing undefined is almost same as no arguments, except for arguments or other edge cases.

littledan commented 4 years ago

I agree with @devsnek , that undefined should be treated the same as no second parameter. On the other hand, I'd expect null to cause a TypeError due to getting the with property from it.

In the current draft in the explainer, you'd use { with: { type: "json" } } as the second argument to import(). We could discuss changing this in a separate issue, if you have concerns about it.

ljharb commented 4 years ago

@devsnek not for import(), or import(a,) would be allowed.

If, however, this proposal is going to make trailing commas in import() allowed, then i agree it should match with function call expectations.

littledan commented 4 years ago

This proposal is going to make trailing commas allowed in import().

devsnek commented 4 years ago

Why not just go all in and use |Arguments|

JLHwung commented 4 years ago

@devsnek I think the concern here is still valid for import(a, b): Supporting SpreadElement requires runtime error check on argument length.

devsnek commented 4 years ago

I guess I don't see any real reason to police the number of arguments passed.

jridgewell commented 4 years ago

I don't understand why we treat import special. Just give it arguments. Who cares if you pass 0 args, 1 arg, or 100. If the first arg can be resolved to a specifier, then you're fine. And if the second arg is provided, and its a real object, you're fine.

That import is a special keyword shouldn't change they way we process the arguments you pass to it. And it certainly shouldn't make import(foo, bar) behave differently if bar resolved to undefined.

ljharb commented 4 years ago

It's not a function tho, and it's very important it not be treated as one. Functions can be passed around, import() can't, and functions - most importantly - do not behave differently based on where they are invoked, while import() can.

jridgewell commented 4 years ago

We invoke it like a function, and it returns a value like a function. Whether the callee is a real value or a syntax form shouldn't affect the way we treat its arguments.

littledan commented 4 years ago

I see I thumbs-upped the comment on the restriction initially, but I'd be OK with weakening it to general |Arguments|, or keeping the two-arg, no-spread restrictions as in the current spec outline. I see this as a post-Stage 2, non-core bikeshed, since there are multiple options that would each be non-fatal, and the task is to figure out which is best.