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 conf.definitionMap as a module #1954

Closed
saschanaz opened this issue Dec 16, 2018 · 10 comments
Closed

Refactor conf.definitionMap as a module #1954

saschanaz opened this issue Dec 16, 2018 · 10 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Dec 16, 2018

definitionMap is not really a configuration, it's a collection of elements set in run-time. Maybe import { definitionMap } from "./dfn"; should be preferred as currently passing conf.definitionMap to functions is a pain.

@saschanaz
Copy link
Member Author

Or maybe modularize conf itself?

@marcoscaceres
Copy link
Member

I agree. We should not be using conf to pass stuff around. That's a (terrible) legacy thing we should put a stop to.

@saschanaz
Copy link
Member Author

@marcoscaceres What should be the module look like? I'm thinking about two choices:

  1. import { conf } from "configuration"; where conf is exactly the object that modules get as a parameter. Easiest to migrate.
  2. import * as conf from "configuration"; conf.get("something"); where the module exposes get() and setDefault() functions. A more encapsulating way.

@marcoscaceres
Copy link
Member

I was thinking the opposite. I don't think we need a module for configuration, as it's always just passed as an argument when a plugin is initialized (which I think makes sense).

But what we should check is where random things get added to conf by a module (e.g., the definitionMap). That's not great. Things like definitionMap should get exported from modules, and not added to conf.

@saschanaz
Copy link
Member Author

as it's always just passed as an argument when a plugin is initialized (which I think makes sense).

It makes sense, but it's still a pain because frequently its functions have to share the config object and the only good way is to pass it as an argument every time.

@marcoscaceres
Copy link
Member

I’m open to exploring the pain points, fwiw.

Another option is to simply access window.respecConfig, as conf is always a pointer to that.

My main concern with the export would be around it being racy. The current design assures that respecConfig is the whatever is available after load.

@saschanaz
Copy link
Member Author

Another option is to simply access window.respecConfig

Accessing global is no-go for me, as one of my ultimate hope is to make ReSpec node-able, in other words, to process documents without a full browser executable (#1469).

My main concern with the export would be around it being racy.

IMO it should probably be okay as the base-runner module controls when should a module run.

@marcoscaceres
Copy link
Member

Ok, let’s play this out a bit. How do you envision user configuration working with the module?

@saschanaz
Copy link
Member Author

I would keep the global respecConfig thing for now but only change how to access it from other modules, probably with get() and setDefault() functions exposed. My concern here is conf.get("name") is more verbose than conf.name 😅

@saschanaz
Copy link
Member Author

(I will try the definition map thing first as we have a consensus.)

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

2 participants