search

dojo / util

Dojo 1 - build utilities. Please submit bugs to https://bugs.dojotoolkit.org/
https://dojotoolkit.org/
Other
60 stars 105 forks source link

Handle loader plugins used together with dojo/has plugin. Fixes #17438 #7

Closed chuckdumont closed 10 years ago

neonstalwart commented 10 years ago

that looks good. thanks.

@rcgill are you happy for it to be merged?

chuckdumont commented 10 years ago

Also, regarding the CLA. I've been informed that as an IBM employee, I'm already covered by IBM's CCLA.

cjolif commented 10 years ago

I confirm @chuckdumont contributions are covered by IBM CCLA.

neonstalwart commented 10 years ago

i think the changes are good to be merged i'm just not sure if i'm overstepping boundaries for me to commit this myself - maybe i'm being too cautious. if someone else with commit privileges (like @cjolif) wants to take responsibility for committing it then it's not up to me to say no.

chuckdumont commented 10 years ago

Note that in moving the function getAmdModule to the bc, I changed the closure scope reference resource to the formal parameter reference referenceModule instead. This can be done because getAmdModule was only ever called with referenceModule === resource.

cjolif commented 10 years ago

I thought I answered to @neonstalwart but apparently not. So if @rcgill does not step in I think you are definitely better positioned than me to merge this as you clearly know that code better than me.

cjolif commented 10 years ago

ok so if nobody has objection I'm going to merge this in master and 1.9.

chuckdumont commented 10 years ago

Thanks Chrisophe!

cjolif commented 10 years ago

While doing testing before merging this is working fine on Node.JS but seems to be causing some trouble on Rhino at least in a test case. @chuckdumont have you tried it on Rhino? @clmath is also looking into find the origin of the issue which might be linked to the changes made to getAmdModule.

clmath commented 10 years ago

I am sorry, there was a "debugger;" statement left in my code and that's why Rhino was not working. After removing it I confirm the code is working well in Rhino and NodeJS.

cjolif commented 10 years ago

ok thanks @clmath I'll run a few more tests and merge.

cjolif commented 10 years ago

merged in 94a0d235fee5303b904c1b2f90d5b3872b92bedd