Skip to content

Conversation

@purusho
Copy link
Contributor

@purusho purusho commented Mar 5, 2015

This PR will fix the loading of controllers for the AMD version.
Sample App's PR : rendrjs/rendr-examples#15

@ingiulio
Copy link
Contributor

ingiulio commented Mar 5, 2015

+1

Copy link
Member

Choose a reason for hiding this comment

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

Looks like realAction is a function (object) and requireAMD expects list of strings as dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Then let's check for the variable type, which will protect us from changes down the road
and will allow more flexible treatment of AMD environment on the server.

@alexindigo
Copy link
Member

👍

@rendrjs/maintainers LGTM

@saponifi3d
Copy link
Contributor

Seems good - can you please add a few tests around this?

@purusho
Copy link
Contributor Author

purusho commented Mar 10, 2015

test added.

Copy link
Member

Choose a reason for hiding this comment

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

Have you tried to run actual app with this change?

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, and it's working.

Copy link
Member

Choose a reason for hiding this comment

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

Cool :)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.41%) to 82.69% when pulling 55ba980 on purusho:master into 7fb33df on rendrjs:master.

@saponifi3d
Copy link
Contributor

Aw yeah! 83% coverage! We're gettin' there!

saponifi3d added a commit that referenced this pull request Mar 11, 2015
Load controller from action for AMD version
@saponifi3d saponifi3d merged commit 36b7923 into rendrjs:master Mar 11, 2015
@purusho purusho mentioned this pull request Mar 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants