Skip to content
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

Unmixing type from other attributes #63

Open
eemeli opened this issue May 27, 2020 · 18 comments
Open

Unmixing type from other attributes #63

eemeli opened this issue May 27, 2020 · 18 comments

Comments

@eemeli
Copy link
Member

eemeli commented May 27, 2020

Having read through the other issues, I note that this part of the proposal isn't exactly accurate:

Another option considered and not selected has been to use a single string as the attribute, indicating the type. This option is not selected due to its implication that any particular attribute is special; even though this proposal only specifies the type attribute, the intention is to be open to more attributes in the future.

As currently envisioned, the type option is indeed special. It defines the loader to use to transform the resource into its imported form. Any other additional attributes are really arguments for that loader.

I mean, the reason there's a need for type in the first place is that we can't trust that a network request for a .json file actually resolves to be a JSON file; type is fulfilling the role that file extensions have traditionally taken.

Making a keyword-based type explicitly special would also provide a way for those attributes not to need to be defined inline and separately for each import, by providing a key to use in a central registry for other attribute defaults.

Finally, the simplicity of a string as value and the power of a with key:value set of attributes are not mutually exhaustive. Why not have both?

import.meta.registerType('custom', loadFunction, { some: 'attributes' })

import foo from './foo' as 'json'
import foo from './foo' as 'json' with foo: 'bar'
import foo from './bar' as 'custom'
import foo from './bar' as 'custom' with some: 'override'

This sort of syntax would provide both simplicity and flexibility, allowing for often-used attribute sets to be controlled centrally, while maintaining the liberty of defining specific values for a single instance.

Overall, the role of module attributes seems rather similar to that fulfilled e.g. by Webpack loaders. This is what the Webpack docs say on this now, based on a few years of experience:

Use module.rules whenever possible, as this will reduce boilerplate in your source code and allow you to debug or locate a loader faster if something goes south.

@littledan
Copy link
Member

We did consider this in various previous drafts. However, we got very strong feedback from TC39 that we should not make type special, and instead put it on a level playing field with other attributes. See notes. cc @bmeck

@bmeck
Copy link
Member

bmeck commented May 29, 2020

I would be fine disambiguating assertion based attributes with evaluator attributes. However, I do not currently see type as a special case of assertions and think that providing special syntactic carve out for its use to be concerning still.

It defines the loader to use to transform the resource into its imported form.

Note: type does not do any transformation and is an assertion attribute and does not alter any behavior except for a form of integrity checking. I would likely have objections to it currently if it did some sort of runtime alteration.

@ljharb
Copy link
Member

ljharb commented May 29, 2020

@bmeck (perhaps you've already done so) but could you elaborate on what other kinds of assertions you foresee applying?

@bmeck
Copy link
Member

bmeck commented May 29, 2020

@ljharb some that come to mind off top of my head

attribute name bikeshed values description
async boolean guard modules that evaluate across multiple ticks. useful for certain scenarios. Service workers actually have this kind of check during their initialization
integrity string for non-cyclic/pinned dependencies it is easier than out of band meta-data. Not a universal solution
declarative boolean guard to prevent top level evaluation (transitive), essentially limiting code to function declarations at their top level. useful for similar reasons as the JSON not evaluating code concern
timeout number a maximum amount to wait before aborting/considering the resource stale. mostly useful for import()
preloaded boolean a boolean that fails eagerly if the resolved module would require a fetch/is not already loaded. mostly useful for IOT

@ljharb
Copy link
Member

ljharb commented May 29, 2020

  • async would prevent importing anything that used TLA?
  • I don't understand declarative. can you elaborate?
  • timeout seems like something you can do with Promise.race; can you help me understand why it would need to be part of the module system?

@bmeck
Copy link
Member

bmeck commented May 29, 2020

@ljharb

async would prevent importing anything that used TLA?

yes, and WASM modules. see things like nodejs/node#33605 (comment) where this is also relevant to node, it also means that you don't have as many parallelism concerns in code like:

import 'has-async-side-effects';
import 'needs-those-effects' with async=false;

I don't understand declarative. can you elaborate?

The concern about JSON is the arbitrary code evaluation on import of that resource, where you could swap the format from JSON -> JS. If you also prevented all side effects from import it would have the same kind of guarantee of not causing side effects. E.G.

export function foo() {
  // code can go here
}

Does not have any JS code that runs on import.

timeout seems like something you can do with Promise.race; can you help me understand why it would need to be part of the module system?

You cannot put a Promise.race inside an import statement.

@ljharb
Copy link
Member

ljharb commented May 30, 2020

Sure, but timeout only really makes sense for dynamic import, as you said.

If “declarative” is a thing, why is the type needed at all?

@bmeck
Copy link
Member

bmeck commented May 30, 2020

@ljharb assertions are just about applying checks/guarantees and the different guarantees are all about being able to specify those clearly. An expectation of JSON as a type is different in nature to expectation of lack of effects. The web is interested in the JSON check currently.

@ljharb
Copy link
Member

ljharb commented May 30, 2020

My understanding, though, is that the actual thing the web is interested in is what "declarative" does - ensuring that something that has no effects when you type the code, can't suddenly start having effects later.

@littledan
Copy link
Member

Note, we're also discussing various possible attributes in other issues such as #8 #43 #46

@littledan
Copy link
Member

The particular web integration privilege escalation issue isn't about side effects or JSON per se but more about the privilege to run arbitrary code. You could imagine side effects which don't have the privilege to run arbitrary code (though my mind runs a little blank), or things which don't have that privilege which aren't JSON (e.g., CSS modules). We also talked about a "noexecute" flag, but ultimately that would also have to be some kind of thing associated with the module on the request's side, not the response.

@Jack-Works
Copy link
Member

we should not make type special

I strongly agree with this. But how do we achieve this?

In future, it would be strange if a host doesn't support type attribute cause major platforms are supporting it. Just like today, it would be strange if a host doesn't have a "url" property on the import.meta. It will be special by the force of the community.

@Jack-Works
Copy link
Member

declarative | boolean | guard to prevent top level evaluation (transitive), essentially limiting code to function declarations at their top level. useful for similar reasons as the JSON not evaluating code concern

I like this idea 🎉

@Jack-Works
Copy link
Member

Jack-Works commented May 30, 2020

You could imagine side effects which don't have the privilege to run arbitrary code

Yes, it does (not strictly).

// hack.js
export function then() {
    console.log('Ah ha')
    return {}
}

// main.js
import('./hack.js', { declarative: true }).then(console.log)
// logs "Ah ha", the side effects.

@ljharb
Copy link
Member

ljharb commented May 30, 2020

@littledan this entire proposal is for the request’s side, not the response’s?

@bmeck
Copy link
Member

bmeck commented May 30, 2020

Things like #63 (comment) make me think that the JSON case is much simpler as a starting point than trying to iron out all possible places where code execution may occur due to things like string property protocols. I think starting with JSON is much simpler than trying to do most of these other attributes. E.G. Similar semantic concerns about then() would also potentially affect any async attribute and need to be thoroughly agreed upon.

@ljharb
Copy link
Member

ljharb commented May 31, 2020

I very much agree that any new attribute needs to be fully thought out and go through the process - I’m not on board with the current host grab bag approach.

@littledan
Copy link
Member

littledan commented Jun 3, 2020

The way modules are served to JS engines is handled by the host; I don't see how we could move more into the JS spec.

It's a core goal of this proposal to enable module types which aren't required to be supported in all JS implementations (e.g., HTML, CSS, and WebAssembly modules).

This proposal tries to make type analogous to other attributes by putting it in the same syntactic position as other attributes, based on feedback from TC39 about this question. At the same time, it puts requirements on type (in particular, when it comes to caching) to minimize divergence among JS implementations/hosts where possible.

This proposal works to require as much as possible in common between platforms. However, there are JS platforms that do not try to align with the web in how their module system works (e.g., XS/TC53), and this proposal works to not overstep those bounds, or prohibit future directions that have been proposed in this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants