Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

Add 'require' flag to preload JS modules #83

Merged
merged 2 commits into from
Mar 28, 2018

Conversation

ooflorent
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Add --require flag (and its alias -r) to preload JS modules. The flag could be added multiple times to load multiple modules.

webpack-serve -r dotenv/env

Breaking Changes

none

Additional Info

The biggest wins with this flag is the possibility to use dotenv and esm packages. Unfortunately, esm does nothing because cosmiconfig uses require-from-string (track progress here cosmiconfig/cosmiconfig#109).

@codecov
Copy link

codecov bot commented Mar 28, 2018

Codecov Report

Merging #83 into master will decrease coverage by 12.2%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #83       +/-   ##
===========================================
- Coverage    92.7%   80.49%   -12.21%     
===========================================
  Files           7        7               
  Lines         274      282        +8     
===========================================
- Hits          254      227       -27     
- Misses         20       55       +35
Impacted Files Coverage Δ
cli.js 97.43% <100%> (+0.66%) ⬆️
lib/bus.js 58.82% <0%> (-41.18%) ⬇️
lib/config.js 65.38% <0%> (-34.62%) ⬇️
lib/options.js 73.56% <0%> (-16.1%) ⬇️
lib/server.js 83.07% <0%> (-4.62%) ⬇️
index.js 91.48% <0%> (-4.26%) ⬇️

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 5fce7a4...3f4bfb2. Read the comment docs.

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

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

this will be a win for folks 🎉 thanks for putting it together

we'll need package-lock.json removed from the commit - that's something we maintain during publish because it's messy

and we'll need a test or two added to cli.js to make sure this doesn't break unexpectedly in the future.

cli.js Outdated
@@ -68,7 +94,7 @@ const { config } = explorer.load() || {};
const options = merge({ flags }, config);

if (cli.input.length) {
[flags.config] = cli.input;
flags.config = cli.input[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change? pretty sure that fails linting due to the prefer-destructuring rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is using ES6 features for nothing. It is a clever trick but does not improve readability and may be confusing for JS beginners. I've reverted this change.

@shellscape shellscape merged commit ae70e96 into webpack-contrib:master Mar 28, 2018
@shellscape
Copy link
Contributor

shellscape commented Apr 2, 2018

@ooflorent I wonder if we could mock require-from-string and intercept the requiring of that module with https://www.npmjs.com/package/mock-require.

What I'm thinking is that we mock that module before requiring cosmiconfig, if the --require flag is used. They're passing the content of the file as well as the file path of the found config file. We could then use that path to require the file so the hooks can do their thing. That's the theory anyhow. Thoughts?

@ooflorent ooflorent deleted the feature/r-flag branch April 3, 2018 09:38
@ooflorent
Copy link
Contributor Author

That could work but it's hacky. Another option is to replace cosmiconfig by something else.

@shellscape
Copy link
Contributor

cosmiconfig really is the best at what it does. I'm not overly opposed to hacks for a development module when that hack is for somewhat-edge cases.

@ooflorent
Copy link
Contributor Author

@shellscape If you PR something, I'll be glad to review it ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants