tc39/proposal-import-attributes

Should we allow string literal as key in condition entries?

JLHwung opened this issue ยท 12 comments

import "./foo.json" if { "type": "json" }

As @littledan mentioned in #77 (review)

[It is] for further consistency with object literals

Since JavaScript implementations are encouraged to reject conditions which are not implemented, as of now "type" is the only valid use case. However, type is always preferred over "type" for code size reasons. I lean to disallow it and revisit in the future if we must.

Besides, the literal property name also includes numeric literals. So even if string literal is supported, the literal property name is still a superset of the condition entry key.

This logic makes sense to me. I'm happy to leave the proposal as is, generalizing the syntax only if we have a strong reason to do so.

i think we should allow string literals. combined with string literals already being used in import syntax, and the arbitrary module namespace identifiers proposal, i think it would be more consistent.

If quotes are disallowed, people will complain about inconsistency; if they're allowed, people will have to make linter rules - and since eslint core no longer does style rules it'll actually be super annoying for the ecosystem to handle that.

I don't feel strongly either way though, as long as we make a decision - I lightly lean towards consistency here.

@ljharb What do you see as the "consistency" option here? I guess they're both consistent, within their own logic.

@dandclark It would be great to have a PR sketching out this option, in case we decide to go this way (just like we have for assert). IMO this should allow " and ' strings, but disallow numeric literals and computed keys before the :

@littledan I'll prep that PR.

@littledan i see consistency as "an object-like thing treats quoted and unquoted identifiers the same" (named import syntax is curly braces but not object-like).

xtuc commented

While I agree that being consistent with object literal is what we want, do we have a concret example that doesn't work without the quotes?

I guess someone could be frustrated if they have this:

import("./foo", { if: { "key1": "value1", "key2", "value2", ... } }) // quotes around keys are fine because it actually is an object literal here

And when they copy/paste to a static import, it's a syntax error:

import "./foo" if { "key1": "value1", "key2", "value2", ... }; // error: unexpected token '"', or something

And then they have to strip the quotes around the keys to make it work.

On the other hand it might be preferable to start with the more restrictive version (only allow IdentifierNames) and only open it up to StringLiterals if this becomes a problem in practice. In any case we're not going to reach complete parity with object literals as long as we're still disallowing NumericLiterals, expressions, etc, so allowing StringLiterals just puts us further in this strange middleground.

There are good points on both sides of this issue, I could really go either way on whether we should take #84.

I would be surprised if I couldn't use an arbitrary LiteralPropertyName in this position.

That would move things further along the object-literal-like spectrum if that's what we want to optimize for.

@littledan You'd stated above that you were not in favor of allowing NumericLiterals, thoughts?

I don't see a really solid reason for/against any of these, but NumericLiteral key names just seem odd to me, especially for 'check' conditions. I'm having trouble thinking of a use case where a host would ever use one as a key name for a check. I guess the argument might be that syntactically it should be allowed for consistency's sake, even if they probably wouldn't be used in practice?

However unlike both StringLiterals and IdentifierNames, NumericLiterals are not currently allowed in any other parts of import statements.

xtuc commented

On one side I would like to restrict it to IdentifierNames only, which will avoid people wanting to use micro syntax again for keys but the dynamic import form will allow that anyway, so maybe we should just keep it as consistent as possible.