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.

Sign up for GitHub

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

add Store#hasModule(path) API #834

Merged
merged 6 commits into from
Mar 25, 2020
Merged

add Store#hasModule(path) API #834

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2533e14
add Store#hasModule(path) API
FranckFreiburger Jun 15, 2017
57744da
fix Store::hasModule(path) return value
FranckFreiburger Jun 15, 2017
09f3197
add unit test for Store#hasModule(path)
FranckFreiburger Jun 15, 2017
338dc9a
Revert "add unit test for Store#hasModule(path)" (reverts commit 09f3…
FranckFreiburger Jul 10, 2017
bf174ed
fix linting issues
FranckFreiburger Jul 10, 2017
53754c9
Merge branch 'dev' of github.com:vuejs/vuex into dev
FranckFreiburger Jan 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/module/module-collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ export default class ModuleCollection {

parent.removeChild(key)
}

isRegistered (path) {
const parent = this.get(path.slice(0, -1))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store.registerModule(['aze', 'rty'], ... // THROW

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this behavior is expected, maybe it's a documentation issue ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@FranckFreiburger FranckFreiburger Jun 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const key = path[path.length - 1]

return parent.hasChild(key)
}

}

function update (path, targetModule, newModule) {
Expand Down
4 changes: 4 additions & 0 deletions src/module/module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ export default class Module {
return this._children[key]
}

hasChild (key) {
return key in this._children
}

update (rawModule) {
this._rawModule.namespaced = rawModule.namespaced
if (rawModule.actions) {
Expand Down
10 changes: 10 additions & 0 deletions src/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,16 @@ export class Store {
resetStore(this)
}

hasModule (path) {
if (typeof path === 'string') path = [path]

if (process.env.NODE_ENV !== 'production') {
assert(Array.isArray(path), `module path must be a string or an Array.`)
}

return this._modules.isRegistered(path)
}

hotUpdate (newOptions) {
this._modules.update(newOptions)
resetStore(this, true)
Expand Down
12 changes: 12 additions & 0 deletions test/unit/modules.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,18 @@ describe('Modules', () => {
store.commit('a/foo')
expect(mutationSpy).toHaveBeenCalled()
})

it('dynamic module existance test', () => {
const store = new Vuex.Store({
})

store.registerModule('bonjour', {
})

expect(store.hasModule('bonjour')).toBe(true)
store.unregisterModule('bonjour')
expect(store.hasModule('bonjour')).toBe(false)
})
})

// #524
Expand Down