caiogondim / webpack-conditional-loader

C conditionals directive for JavaScript
https://npm.im/webpack-conditional-loader
MIT License
110 stars 25 forks source link

Regex m flag #3

Closed Darmikon closed 7 years ago

Darmikon commented 7 years ago

@caiogondim hi. Please review my pr. While I was testing your loader I found a bug. #endif doesn't work and all code after #if was commented without any stop. Now it seems to work fine. You can test it from this fork: "webpack-conditional-loader": "git+https://git@github.com/Darmikon/webpack-conditional-loader.git"

caiogondim commented 7 years ago

Hi @Darmikon Can you submit a failing test case? They are all on test/ folder.

Darmikon commented 7 years ago

@caiogondim I can't run your test suite on windows. Without multiline flag in regex I got something like this:

let a = 1
//// #if 1 === 1
//a = 2
//// #endif
//
//module.exports = a

After first #if all code is commented. And your function doesn't react on #endif at all. m flag fixed it and at the moment I'm using fork version.

caiogondim commented 7 years ago

You're using Windows. Now it makes sense. Take a look here: https://github.com/caiogondim/webpack-conditional-loader/blob/master/src/index.js#L100

Multiline regex is not a fix for it. We have to consider Windows newline as well.

As a test, save your file with Unix newline and run webpack again. It should work.

caiogondim commented 7 years ago

This seems to be a good fix: https://nodejs.org/api/os.html#os_os_eol

Darmikon commented 7 years ago

@caiogondim os.EOL works fine instead of m flag. I've updated PR

caiogondim commented 7 years ago

Merged. New version already available: https://www.npmjs.com/package/webpack-conditional-loader

caiogondim commented 7 years ago

You commits are here: https://github.com/caiogondim/webpack-conditional-loader/commits/master Had to rebase. Guess the GitHub UI got lost :/

caiogondim commented 7 years ago

Make sure it works for you and let me know.

Darmikon commented 7 years ago

@caiogondim I've just updated my boilerplate and it works fine with new "webpack-conditional-loader": "1.0.10" version. Thanks for fast feedback.

caiogondim commented 7 years ago

You're welcome 😄 Thanks for the bug report.