-
Notifications
You must be signed in to change notification settings - Fork 307
Dependency injection #447
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
Merged
Merged
Dependency injection #447
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
23fd554
Allow an instance of templateAdapter to be supplied
jmerrifield cf3b47b
Allow ClientRouter class to be supplied directly
jmerrifield ea60622
Pass instances in options, not attributes, so we don't try and JSON-i…
jmerrifield 07ea9ad
Pull router module name into variable so browserify doesn't follow it
jmerrifield 1ae60e7
Fix references to options if not supplied
jmerrifield f932400
Test coverage for new functionality
jmerrifield File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| module.exports = function (options) { | ||
| return { | ||
| name: 'Test template adapter', | ||
| suppliedOptions: options | ||
| }; | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
requirein the browser? Looks like something very specific to browserify.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.
it was already doing this on line 13? not sure how it's terribly different from the original functionality there. (Totally agree with you though.. 😞 )
Any suggestions to fix it up? my only thought is
this.modelUtils._requireAMD(defaultRouterModule)instead.@alexindigo #454
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.
Before it had "hardcoded" default router path, so
r.jswas able to augment it and with this change it makes it invisible forr.js.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.
Btw, looks like assign path to a variable is not essential to this change. So we might revert that particular line to the previous state. While we're thinking on better implementation.
In general IoC should make things better, but here it went sideways :)
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, balls. sorry about that guys. is the requirejs example updated so i can use it as a test bed for this stuff?
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.
@saponifi3d we have this PR rendrjs/rendr-examples#15 to make it work with Rendr
1.0.1There 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.
Hmm, that doesn't seem to fix it for me, i'm getting an error on this line https://github.com/rendrjs/rendr/blob/master/shared/base/router.js#L184 - it looks like
realActionis a string not the action that is expected.In the meantime i'm going to revert the commit that caused the breakage and push out
1.0.4(i think it's that version?) so it's in a working state for everyone.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.
@saponifi3d just tested the sample app (05_requirejs) and it seem to be working with the latest version of Rendr
1.0.2but we still see therequireissue.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, I haven't merged / released those changes yet for the require. I just did a fresh install and tried the example with 1.0.1 and it didn't work. Its breaking on the real action of the controller on a client-side route. Until there's a working exple there it will be hard for me to make a proper fix.
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.
The example doesn't work with
1.0.1because we need changes from those two PR:#448
#450