-
Notifications
You must be signed in to change notification settings - Fork 14
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
Redux Injector (Enables bindings between redux and dojo 2 widget-core) #2
Conversation
Needs tests |
Codecov Report
@@ Coverage Diff @@
## master #2 +/- ##
======================================
Coverage ? 90%
======================================
Files ? 1
Lines ? 10
Branches ? 1
======================================
Hits ? 9
Misses ? 0
Partials ? 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the thought on how dependencies are managed for interop
package.json
Outdated
"grunt": "~1.0.1", | ||
"grunt-dojo2": "latest", | ||
"intern": "^3.4.6", | ||
"typescript": "~2.4.2" | ||
}, | ||
"dependencies": { | ||
"redux": "^3.7.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, anyone using interop
is going to have to pull in redux
? Would it make sense to just have it as a dev dependency and then the end user chooses what sort of dependencies/parts of interop they are using?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah perhaps that is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this, perhaps they do have to pull in redux... because otherwise the more likely mistake will be they don't install redux at all!
After all it will only get included in the bundle if they actually use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I can see both sides of it... but, it means that this will grow and grow and grow in dependencies whenever it is installed as we increase the capabilities of the package. I wasn't planning on adding dojo
and dijit
as dependencies when we introduce the DijitWrapper
. It would just seem to me that it would be "bring your own production libraries" sort of thoughts, with the code here as loosely coupled as possible.
After all, the build will error if they don't install it. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a peer dependency? seems like that would be the most appropriate... I think you'd actually be okay to add dojo
and dijit
too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotta ❤️ peerDependency
... only thing is if I want to use DijitWrapper
then react
wouldn't be a dependency at all. There is no easy way to deal with that, except for maybe optionalDependencies
?
Type: feature
The following has been addressed in the PR:
Description:
Adds a redux injector that can used to bind a redux store and dojo 2 widgets using the
Container
pattern.Define an injector in the registry and then use
Container
to inject the store.Example Usage:
Resolves #1