Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #12 Support configurable tokens for the gulp module tagger #561

Merged
merged 2 commits into from
Aug 17, 2016
Merged

Fix #12 Support configurable tokens for the gulp module tagger #561

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
23ec8a5
Add failing test
doug-wade Aug 12, 2016
059cea6
Fix #12 Support configurable tokens
doug-wade Aug 12, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/react-server-gulp-module-tagger/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,19 @@ var replace = require("gulp-replace")
// - "__LOGGER__"
// - "__LOGGER__({ /* options */ })"
var isWindows = ('win32' === process.platform)
, REPLACE_TOKEN = /(?:__LOGGER__|__CHANNEL__|__CACHE__)(?:\(\s*(\{[\s\S]*?\})\s*\))?/g
, DEFAULT_REPLACE_TOKEN = tokenToRegExp("__LOGGER__|__CHANNEL__|__CACHE__")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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 = {});
var REPLACE_TOKEN;
if (config.token) {
REPLACE_TOKEN = tokenToRegExp(config.token);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new diff

} else {
REPLACE_TOKEN = DEFAULT_REPLACE_TOKEN;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it masks the outer REPLACE_TOKEN?

Needs a different name inside this function, right?

var token = REPLACE_TOKEN;
if (config.token) {
    token = rokenToRegExp(config.token);
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we want it to replace the outer REPLACE_TOKEN.

Copy link
Contributor

Choose a reason for hiding this comment

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

But... don't we still want the value of the outer REPLACE_TOKEN as a default if config.token isn't passed in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, which is why the if (config.token) { ... } check guards against replacing it

Copy link
Contributor

Choose a reason for hiding this comment

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

But the var REPLACE_TOKEN declares a new variable inside the function that masks the outer variable, making it no longer accessible...?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, it's been updated again since this comment thread started. :)

config.basePath = module.filename.replace(THIS_MODULE,'');
return forEach(function(stream, file){
return stream.pipe(replace(REPLACE_TOKEN, (match, optString) => {
Expand All @@ -24,3 +30,7 @@ module.exports = function(config) {
}));
});
}

function tokenToRegExp(token) {
return new RegExp("(?:" + token + ")(?:\\(\\s*(\\{[\\s\\S]*?\\})\\s*\\))?", 'g');
}
3 changes: 3 additions & 0 deletions packages/react-server-gulp-module-tagger/test/fixtures/custom-token/actual.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
var logger = require('react-server').logging.getLogger(__OZZIE_ALONSO__);
var fooLogger = logging.getLogger(__OZZIE_ALONSO__({ label: "foo" }));
var barLogger = logging.getLogger(__OZZIE_ALONSO__({ label: "bar" }));
3 changes: 3 additions & 0 deletions packages/react-server-gulp-module-tagger/test/fixtures/custom-token/expected.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions packages/react-server-gulp-module-tagger/test/fixtures/custom-token/gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
const gulp = require('gulp');
const tagger = require('../../..');

gulp.task('default', () => {
gulp.src('actual.js')
.pipe(tagger({
token: "__OZZIE_ALONSO__",
Copy link
Contributor

Choose a reason for hiding this comment

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

image

}))
.pipe(gulp.dest('build'));
});