Fix #12 Support configurable tokens for the gulp module tagger #561
Conversation
|
builds on #560 |
|
The stack's getting too tall here. Will review when #560 lands. |
| , THIS_MODULE = isWindows | ||
| ? /(?:[^\\]+\\node_modules\\)?react-server-gulp-module-tagger\\index\.js$/ | ||
| : /(?:[^\/]+\/node_modules\/)?react-server-gulp-module-tagger\/index\.js$/ | ||
|
|
||
| module.exports = function(config) { | ||
| config || (config = {}); | ||
| var REPLACE_TOKEN; | ||
| if (config.token) { | ||
| REPLACE_TOKEN = new RegExp("(?:" + config.token + ")(?:\\(\\s*(\\{[\\s\\S]*?\\})\\s*\\))?", 'g'); |
There was a problem hiding this comment.
Don't like having the regexp machinery duplicated...
Maybe this could be in a function, and the default could be defined using it?
Like:
DEFAULT_REPLACE_TOKEN = tokenToRegexp("__LOGGER__|__CHANNEL__|__CACHE__")There was a problem hiding this comment.
I didn't reuse the second capture group based on this advice from MDN
The literal notation provides compilation of the regular expression when the expression is evaluated. Use literal notation when the regular expression will remain constant. For example, if you use literal notation to construct a regular expression used in a loop, the regular expression won't be recompiled on each iteration.
The constructor of the regular expression object, for example, new RegExp('ab+c'), provides runtime compilation of the regular expression. Use the constructor function when you know the regular expression pattern will be changing, or you don't know the pattern and are getting it from another source, such as user input.
The benefit of using a pre-compiled regex is ~10% -- if the code reuse tradeoff seems worth the lost performance in the default case, I'm more than happy to make the change.
There was a problem hiding this comment.
Yeah, that's a good consideration. I think in this case we can have our cake and eat it, too, though. So long as DEFAULT_REPLACE_TOKEN stays at the package level where it is, the penalty for running it through the RegExp constructor will only be paid once. After that it's just a compiled RegExp. What we want to avoid for performance reasons is new RegExp(...) at run time.
| @@ -6,13 +6,17 @@ var replace = require("gulp-replace") | |||
| // - "__LOGGER__" | |||
| // - "__LOGGER__({ /* options */ })" | |||
| var isWindows = ('win32' === process.platform) | |||
| , REPLACE_TOKEN = /(?:__LOGGER__|__CHANNEL__|__CACHE__)(?:\(\s*(\{[\s\S]*?\})\s*\))?/g | |||
There was a problem hiding this comment.
@gigabo like this?
| , THIS_MODULE = isWindows | ||
| ? /(?:[^\\]+\\node_modules\\)?react-server-gulp-module-tagger\\index\.js$/ | ||
| : /(?:[^\/]+\/node_modules\/)?react-server-gulp-module-tagger\/index\.js$/ | ||
|
|
||
| module.exports = function(config) { | ||
| config || (config = {}); | ||
| if (config.token) { | ||
| REPLACE_TOKEN = tokenToRegExp(config.token); |
There was a problem hiding this comment.
So... it looks like this replaces the default. If I use the module tagger once with config.token and then later once without I'll get the previous invocation's config.token value instead of the default?
|
Looks awesome to me! 🤘 Thanks for humoring my obsession with factoring out the regexp line noise @doug-wade. 😅 |
Support configurable tokens for the gulp module tagger. Fixes #12