Feature/replace injector pattern with modules #85
Conversation
| import routes from './routes'; | ||
|
|
||
| Vue.use(VueRouter); | ||
|
|
||
| let router = null; | ||
|
|
||
| const getRouter = () => { |
There was a problem hiding this comment.
Are you aware that you are changing the behaviour of the running code with this change(s)?
Previously, the code in this function would only be executed when first called. Now it's executed when required (first thing). So will loose some control.
There was a problem hiding this comment.
I see that the behaviour is changed but not how it affects the implementation, do you have an example of lost control?
There was a problem hiding this comment.
Let's say the router setup needs a value that is set in another file. You would like to control the order of how things are executed. Having to rely on changing the import order to (hopefully) change the order of execution is error-prone and unclear.
Explicitly calling functions to specify the order of execution gives you more control.
| const configManager = new ConfigManager(); | ||
| configManager.init(config.config, config.environment); | ||
|
|
||
| export default configManager; |
There was a problem hiding this comment.
Why is this file located in the util folder and not in the config folder?
There was a problem hiding this comment.
Fair point. It used to be created and registered in util/setupInjects.js, I’ve separated the ‘injectables’ into separate files but felt relocating/refactoring should be another issue.
|
Keep in mind that when using default exports, you cannot really unit test files that import those if you need to mock them (unless you have require hooks). |
| @@ -1,6 +1,4 @@ | |||
| const Params = { | |||
| export default { | |||
There was a problem hiding this comment.
We had all enums like this in vue skeleton but after a comment of Narie we changed it to the const with the seperate export default. It had something to do with webstorm. I am not a big fan of a direct export default. I almost always use the const with the separate export default. So when you read the code you understand what is exported. In this case you have to look at the filename but doesn't always work because you can have multiple exports in one file.
There was a problem hiding this comment.
Yeah, webstorm has/d better autocomplete / autoimport if the const has the same name as the file/import name.
There was a problem hiding this comment.
Fair point on the autocompletion, I did not consider this.
Replaced the injector pattern with default exports and removed injector related files.
export default getX => xnow becomesexport default xand the get/setValue(enum) injectors are removed.