add Store#hasModule(path) API #834
Conversation
src/store.js
Outdated
| assert(Array.isArray(path), `module path must be a string or an Array.`) | ||
| } | ||
|
|
||
| this._modules.isRegistered(path) |
There was a problem hiding this comment.
We must return the result of this expression.
| @@ -53,6 +53,14 @@ export default class ModuleCollection { | |||
|
|
|||
| parent.removeChild(key) | |||
| } | |||
|
|
|||
| isRegistered (path) { | |||
| const parent = this.get(path.slice(0, -1)) | |||
There was a problem hiding this comment.
I realized this line can throw an error if the parent module does not exist.
To handle this, we also need to add existance check in get method.
There was a problem hiding this comment.
yes,
store.registerModule('hi' ...
expect(store.hasModule('hi')).toBe(true)
expect(store.hasModule(['hi','big'])).toBe(false)
expect(store.hasModule(['hi','big', 'world'])).toBe(false) // THROW
expect(store.hasModule(['bonjour'])).toBe(false)
There was a problem hiding this comment.
store.registerModule(['aze', 'rty'], ... // THROW
There was a problem hiding this comment.
maybe this behavior is expected, maybe it's a documentation issue ?
There was a problem hiding this comment.
But the purpose of this feature is to check existance of module, isn't it? I think the hasModule should return false in that case.
There was a problem hiding this comment.
yes, this feature is to check existence of module, but maybe not the existence of sub-module.
It's quite like mkdir and mkdirp
But I understand your point of view
There was a problem hiding this comment.
I'm not sure why this method is semantically related with mkdir. They are used on different platform (Web/CLI) and also are different kind of task (hasModule checks a module while mkdir creates a directory).
If the parent module does not exist, it's clear that its child module also does not exist. So why hasModule need to throw in that case? 🤔
There was a problem hiding this comment.
Trust me, I am not trying to create modules with mkdir 😃
I just wanted to say that the responsibility lies with the caller to ensure the correctness of the arguments.
|
|
At this point, |
|
What prevents it from being merged? |
# Conflicts: # test/unit/modules.spec.js
|
@ktsn Can this be merged? Or does this PR need more work? |
|
@ktsn a year has passed. Can we merge this already? Please :) |
|
@yyx990803 any timeline on merging this? |
|
@ktsn @yyx990803 This would also be really helpful to my team can we get this merged in and released? |
|
2020 now, this is such a small pull request. Just merge it already! |
* add Store#hasModule(path) API * fix Store::hasModule(path) return value * add unit test for Store#hasModule(path) * Revert "add unit test for Store#hasModule(path)" (reverts commit 09f3197.) Add a new test for Store#hasModule() * fix linting issues
No description provided.