Keep global require's toUrl and toAbsMid methods intact

dojo / cli-build-webpack

🚀 **DEPRECATED** Dojo 2 - cli command for building applications

http://dojo.io

Other

4 stars 19 forks source link

Keep global require's toUrl and toAbsMid methods intact #47

Closed maier49 closed 7 years ago

maier49 commented 7 years ago

Type: bug

The following has been addressed in the PR:

[ ] There is a related issue
[x] All code matches the style guide
[x] Unit or Functional tests are included in the PR

Description:

Currently, if code is calling require.toUrl, that will trigger the expression require condition and that module will have require = function() { return '${path}'; } prepended to it. However, this function obviously doesn't have a toUrl property so the call will fail. Checking for usages of require.toUrl and not replacing require at all is problematic because webpack will still replace it with __webpack_require__ which doesn't have a toUrl function either.

It seems like there should probably be a better way to handle this. But this does work for todoMVC at least.

codecov-io commented 7 years ago

Current coverage is 99.59% (diff: 100%)

No coverage report found for master at 6269ae6.

Powered by Codecov. Last update 6269ae6...343632a

mwistrand commented 7 years ago

Where is globalObject.require being set? If an application is being built with webpack and therefore using its internal loader, there would be no require in the global namespace.

maier49 commented 7 years ago

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.

matt-gadd commented 7 years ago

I don't think we should land this in its current form for the following reasons:

The current require code in cli-build gives context require support to webpack without an AMD loader giving parity in functionality. For toUrl and toAbsMid in this, that would not be the case which would be confusing in my opinion.
This boilerplate would be added to every point a contextual require is made (regardless of whether .toUrl or .toAbsMid is used) which seems like a overhead in bundle size.
You can actually use require.toUrl with a contextual require in AMD - which this doesn't support.

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 require.toUrl?

maier49 commented 7 years ago

@matt-gadd I agree with that reasoning.

It does bring up one other concern though. This code is currently triggering on usage of require.toUrl. If it actually was a contextual require it will still be replaced with a function without a toUrl method, meaning that using require.toUrl with a contextual require is not supported by the current version either. Unless I'm missing something in how that would work.