dojo / cli-build-webpack

🚀 **DEPRECATED** Dojo 2 - cli command for building applications
http://dojo.io
Other
4 stars 19 forks source link

Fix: use correct banner.md path when ejecting. #151

Closed mwistrand closed 7 years ago

mwistrand commented 7 years ago

Type: bug

The following has been addressed in the PR:

Description:

When ejecting, use the correct file path for the banner used by the webpack banner plugin so that the build can pass without "file not found" errors. Verified with a local app created with $ dojo create.

Resolves #145

codecov[bot] commented 7 years ago

Codecov Report

Merging #151 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #151   +/-   ##
=======================================
  Coverage   96.03%   96.03%           
=======================================
  Files          11       11           
  Lines         631      631           
  Branches      133      133           
=======================================
  Hits          606      606           
  Misses         13       13           
  Partials       12       12

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2a7696c...ef9e4c4. Read the comment docs.

mwistrand commented 7 years ago

@kitsonk Currently the webpack config has no tests, in part because it resolves all paths and instantiates all plugins on load. If we wanted to include tests for it, we could perhaps try exporting a createConfig method that encapsulates 100% of the logic, and then change the default export to export default createConfig();

kitsonk commented 7 years ago

Or use vm.runInContext to stub the necessary context and evaluate the result is as expected. No changes then to the structure of the code.

mwistrand commented 7 years ago

Thanks, @kitsonk. I've been working on a test for this, but am hung up on TypeError: require.resolve is not a function errors. Passing in require works fine, but it cannot register the existence of require.resolve (even when stubbing out require). Based on how TS constructs its UMD wrapper, at the very least the following must be passed in:

const exports = {};
const context = vm.createContext({
    exports,
    module: { exports },
    require
});

On top of that, I've needed to jump through additional hoops to ensure that modules are loaded correctly (./ in some cases, @dojo/cli-build-webpack with a falsy DOJO_CLI env flag). If you have any suggestions, I'd appreciate hearing them.

kitsonk commented 7 years ago

Assuming you are running under the Dojo 2 loader for intern tests, don't you want require.nodeRequire to be passed in as require into a context? I believe using mockery at that point should allow you to mock any other requirements that you want/need to stub out.

matt-gadd commented 7 years ago

can you explain the separation of Sandbox and MockModule a little? I'm struggling to get the intent (ie a lot of "mocking" is going on in Sandbox now?). If there is no real reason behind the separation can MockModule just be removed?

mwistrand commented 7 years ago

@matt-gadd MockModule actually mocks the module being tested, but that's not what these tests need to do. Sandbox is perhaps a misleading name, and perhaps I'm just overthinking this, but I removed the sandboxing/dep mocking into a separate module since that's all I needed for these tests.

mwistrand commented 7 years ago

@matt-gadd I ended up removing Sandbox in favor of adding a start method directly to MockModule. MockModule in this case is still not being used to mock the config, but at least we have only one module to deal with when writing tests.