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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sidvishnoi
Copy link
Member

@marcoscaceres
Copy link
Contributor

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
Copy link
Member Author

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 w3c@a0a4f95, 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
Copy link
Member Author

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

@saschanaz
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Collaborator

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

@marcoscaceres
Copy link
Contributor

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
Copy link
Collaborator

saschanaz commented Mar 12, 2021

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
Copy link
Contributor

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
Copy link
Collaborator

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

Gpapi13
Gpapi13 approved these changes Mar 13, 2021
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

Successfully merging this pull request may close these issues.

4 participants