refactor(core/base-runner): replace conf.state with run(conf, state)

[sidvishnoi]

See https://github.com/w3c/respec/pull/3372#discussion_r590936033

[marcoscaceres]

I'm really sorry, but I'm still struggling to understand this :(

I'm confused as to why prepare() would add a property to state, which is then read by run()? Why not just store state as an internal variable and just read it when it needs it?

Similarly, if there was a case where "plugin A" needs to wait on "plugin B" to finish, why not just:

// plugin B
export const readyPromise;

That then "plugin A" just imports and waits on when it needs it?

// plugin A
import readyPromise as pluginAReady from "plugin A";

function run() {
   await pluginAReady;
}

Clearly, I'm missing something?

[sidvishnoi]

Why not just store state as an internal variable and just read it when it needs it?

I find global state cleaner compared to imported state, and I'm not sure of the benefits of importing state over this approach. A global state container also means our modules are more alike to pure functions (ignoring the state of document), so we can test them as individual units.

That then "plugin A" just imports and waits on when it needs it?

This is what I'm avoiding by adding a global state container. When we import state, we get a convulated running order - where dependencies are defined not only by plugin order in a profile, but also imports. With a global state, only a plugin late in profile can access result of plugin that ran before it.

It also leads to a reduction of some unnecessary async-await like in https://github.com/w3c/respec/pull/3319/commits/a0a4f956b82280ca3eb2d5aa805078e3a74c809c, where's resolveRef async only because of a promisified dependency but isn't doing some async work in reality (in this case, it's fixed by injecting that dependency as an arg, making it explicit that biblio is required by resolveRef).

[sidvishnoi]

@saschanaz what do you think? Me and Marcos have different opinions on how to best share state across modules.

[saschanaz]

I wonder we could use the newer new Plugin() pattern by creating an instance before prepare() and add state to the instance, because I don't like passing conf: #2749.

1. Create instances
2. Prepare calls
3. Actual runs

[sidvishnoi]

Also see: https://github.com/w3c/respec/issues/2835

I don't have a strong opinion™️ on Plugin class, but I have a weak preference for functions over methods, when functions are already encapsulated within modules (i.e. a module file is like a class). Passing args often is somewhat ok with me.

[saschanaz]

Yeah, that was partly also for #1469 where module-level states were harmful.

[marcoscaceres]

I has the opinions™️ about new Plugin(), but we can describe that separately.

The immediate concern I have is about passing state VS import/export state.

[saschanaz]

AFAICT .fetchPromise does not need to be shared across modules, right? For such case I think it's best to be within the module.

For states that need to be shared, the approach I preferred for #2187 was to add them to ReSpec instance. That way we explicitly get the list of shared things which should be more manageable.

[marcoscaceres]

add them to ReSpec instance.

The problem then is that they become public, which can be a foot gun for third party code. I'm trying to avoid another -common situation, where people start trying to hook into things they probably shouldn't be touching or overriding.

[saschanaz]

Agreed, and I guess Symbol() will be helpful in that case 👀