-
-
Notifications
You must be signed in to change notification settings - Fork 384
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
base: main
Are you sure you want to change the base?
Conversation
I'm really sorry, but I'm still struggling to understand this :( I'm confused as to why 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? |
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
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 |
@saschanaz what do you think? Me and Marcos have different opinions on how to best share state across modules. |
I wonder we could use the newer
|
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. |
Yeah, that was partly also for #1469 where module-level states were harmful. |
I has the opinions™️ about The immediate concern I have is about passing |
AFAICT 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. |
The problem then is that they become public, which can be a foot gun for third party code. I'm trying to avoid another |
Agreed, and I guess Symbol() will be helpful in that case 👀 |
See https://github.com/w3c/respec/pull/3372#discussion_r590936033