Keep global require's toUrl and toAbsMid methods intact #47
Conversation
Current coverage is 99.59% (diff: 100%)
|
| @@ -83,7 +83,17 @@ export default class DojoLoadPlugin { | |||
| compilation.moduleTemplate.plugin('module', (source: any, module: any) => { | |||
| if (module.meta && module.meta.isPotentialLoad) { | |||
| const path = stripPath(basePath, module.userRequest); | |||
| const require = `var require = function () { return '${path}'; };`; | |||
| const require = `var require = (function () { | |||
| var globalScope = typeof window === 'undefined' ? global : window; | |||
There was a problem hiding this comment.
We do seem to have several ways in Dojo 2 of getting a reference to the global, whether it is https://github.com/dojo/shim/blob/79882ec5c4fa78eb91171e7ff42ace275aa68562/src/support/global.ts or https://github.com/dojo/loader/blob/master/src/loader.ts#L8 . Maybe we want to normalize this?
There was a problem hiding this comment.
Normalizing the usage definitely seems like a good idea. I've updated the usage here to match the loader example, since that was more or less equivalent to what this was already doing but more thorough.
| @@ -62,7 +62,17 @@ describe('core-load', () => { | |||
| })[0]; | |||
|
|
|||
| assert.instanceOf(source, ConcatSource, 'A new `ConcatSource` is created.'); | |||
| assert.strictEqual(source.source(), `var require = function () { return '/root/path/src/module'; };\n`, | |||
| assert.strictEqual(source.source(), `var require = (function () { | |||
| var globalScope = typeof window === 'undefined' ? global : window; | |||
There was a problem hiding this comment.
Same comment/question here about reference to global.
|
Where is |
|
This came up within the context of building our tests. When we run the tests with Intern it's still going to use its own loader so there will be a require in the global namespace.
If there isn't one this effectively reverts back to its original behavior.
|
|
I don't think we should land this in its current form for the following reasons:
When we move to the version of intern that is loaderless, we won't have an AMD loader anyway, so it seems like in the mean time we might be better just changing the way we access an html page in todo-mvc to not use |
|
@matt-gadd I agree with that reasoning. It does bring up one other concern though. This code is currently triggering on usage of |
Type: bug
The following has been addressed in the PR:
Description:
Currently, if code is calling
require.toUrl, that will trigger theexpression requirecondition and that module will haverequire = function() { return '${path}'; }prepended to it. However, this function obviously doesn't have atoUrlproperty so the call will fail. Checking for usages ofrequire.toUrland not replacing require at all is problematic because webpack will still replace it with__webpack_require__which doesn't have atoUrlfunction either.It seems like there should probably be a better way to handle this. But this does work for todoMVC at least.