refactor: rewrite tests to use webpack compiler, drop node 8

[beeequeue]

• Replaces the {} replacement with "MISSING_ENV_VAR" which seems to not error, which the old one started doing for some reason
• Uses the Webpack target to decide to stub process.env or not.
  For now it will not do it for Node and Electron targets
• Rewrite all the tests to use the webpack compiler, and then testing on the output files
• Drops support for Node 8 as the tests can't run on it because of BigInt not existing

You can test it for yourselves by using a yarn resolution:

  "resolutions": {
    "dotenv-webpack": "npm:@beequeue/dotenv-webpack"
  },

Fixes #271
Closes #289

CI runs: https://github.com/BeeeQueue/dotenv-webpack/actions?query=branch%3Afix-stubbing

[mrsteele]

Thanks for checking this out @beeequeue, any chance you ran this with all the supported versions of Webpack? Would love to keep it compatible with the other versions. Also, any chance we can add some tests in for this new check?

[beeequeue]

Turns out that the CI actions are not being run on PRs, and all the tests are failing! 😄

For this solution to be testable I think we need to set up a webpack compiler and compile stuff in the tests, so I'll have to do that.

I think it would make more sense to drop support for webpack <4 considering those versions are 4 years old at this point, and any changes made at this point will most likely be to support or use new features in webpack.

[mrsteele]

As long as we have at least 4 and 5 I think I'd be good. I imagine supporting those older versions would be challenging?

[mrsteele]

(I went back and looked and webpack@v3 was last updated May 2018)

[beeequeue]

More about it being time consuming to (manually!) test for them, and then creating workarounds if something is found when no one should really be using them.

[mrsteele]

@beeequeue would it make more sense to make the user make that distinction? #289 <- That PR puts a flag in the config to opt out of the stub but curious on your thoughts.

[beeequeue]

I think having the option to override this functionality is a very good idea, and having sensible defaults on when to do it ourselves.

Or maybe it would be even better to extract that to another plugin, ProcessEnvStubPlugin that will take care of this...

Another benefit of merging this PR is that the tests will be better, as in they are testing the real behavior of webpack as the tests will use the compiler now

[beeequeue]

Since the test refactoring is very large I'm changing this PR to be about the tests and dropping Node only, and opening another one for the fixes afterwards.

[mrsteele]

This is looking really good. Any chance we can add the stub functionality in as well as drop the deprecated argument? I think once those are in this is a perfect release candidate for #363

[beeequeue]

I extracted the check to its own function, made it not run on webpack <5, and changed the check so it will not stub if the target includes node, rather than starts with it.

I have no idea what the various other node targets are used for or if they have process, but if something breaks for the few people who use those I'm sure they'll come screaming...

[mrsteele]

Everything I have tested so far looks good, seems to be right where it needs to be. Really great work and excited to get this out there!

On the browser list... target is SO confusing and not seeing a lot of documentation about the inner workings.

Looking at the source-code is a little more illuminating.

Would it make more sense to use getTargetsProperties and if !node then we stub MISSING_ENV_VAR since that is ultimately the problem?

[beeequeue]

I think the current checks are good enough.

browserslist is a special case that we can't account for, since it can target whatever it wants (chrome 89, node 14, es5), but it's mainly used for browsers so we can assume it as browser for now.

[mrsteele]

At minimum node would need to be added, and I think it just begs the question on what others should be added. I personally don't know enough about the other targets to make that decision.

Do you think it would be safer to just drop the implicit route so we don't overstep some of those grey-area targets?

[beeequeue]

As I said I think the current implementation will work for 90% of users, if not more.

If someone has a very special setup they are most likely an advanced user and should be able to debug and find our solution and how to disable it if they need to.

Sadly getTargetsProperties is not a public API, so using it is not recommended

[beeequeue]

Sadly recursive: true was added in Node 12, and as such our Node 10 tests fail if we use it.

[mrsteele]

Looking really good. I'm about ready to push this button. Anything else left?

[beeequeue]

Not that I can think of. I'm sure there'll be something after it's out though. That's just how it is 😄

[github-actions]

🎉 This PR is included in version 7.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀