Provide path to replace(); undefined value -> empty replacement string #1

cxw42 · 2018-09-08T16:02:00Z

Thanks for this plugin! I am relatively new to brunch. My use case is to replace __filename with the name of the file being processed, and this plugin and PR are the most direct way I could find to do it. My example is in cxw42/brunch-test.

This PR introduces two related changes, plus documentation, tests, and cleaning to pass eslint checks.

The file.path parameter is given to the replacement function.
If no value key is given, the replacement string is '' rather than the undefined provided by JSON.stringify(). That way, if your custom replacement function is going to use the path argument anyway, the behaviour is well-defined even if you don't specify a replacement string.

Note: this PR also updates the dependency on eslint from 3.x to 4.x. This is because eslint-config-brunch@1.0.0 now requires eslint@^4. I see that you did not check in the package-lock.json, so I also did not do so.

Thanks for considering this PR!

Two related changes, plus eslint cleaning, dependencies, and tests. - If no `value` key is given, the replacement string is '' rather than the 'undefined' provided by JSON.stringify(). - The `file.path` parameter is given to the replacement function. Note: this updates the dependency on eslint from 3.x to 4.x. This is because eslint-config-brunch@1.0.0 now requires eslint@^4.

- `lib/simpletest` now requires `./lib/inner` - Replace `__filename` with the filename using tkesgar/replacer-brunch#1

I tried replacing `__filename` in the wrapper at first, but that didn't work (and I'm not sure why). Now that tkesgar/replacer-brunch#1 is taking care of that, I put the wrapper back in, and it seems to be doing OK.

cxw42 · 2018-09-08T16:32:32Z

index.js

@@ -6,12 +6,18 @@ class BrunchReplacer {

    // Set defaults for config.
    if (!this.config.dict) this.config.dict = [];
-    if (!this.config.replace) this.config.replace = (str, key, value) => str.replace(key, value);
+    if (!this.config.replace) {


eslint flagged the missing {}, so I added them.

cxw42 · 2018-09-08T16:33:15Z

test.js

@@ -41,12 +41,18 @@ describe('Plugin', () => {
      dict: [{key: '__KEY__', value: '__VALUE__'}],
    }}});

-    it('should replace key with value', () => {
+    it('should not replace when key is absent', () => {


I think the test was correct but the description was copied and pasted from elsewhere. I updated the description to match the test.

tkesgar · 2018-09-09T06:45:58Z

Hello there, thanks for the pull request! I'm currently away from computer over the weekend, but from here the PR looks okay.

Using empty string as default for value seems fair game, since I can't think any use cases where people want to replace strings with undefined (they can use 'undefined' string anyway :P).

For the file path, I am okay with the feature; it is useful and doesn't seem to break anything. However, there are a few things to consider:

The path should not be absolute as it could pose a security risk. Hopefully Brunch already provides relative paths, but if it isn't then we should make the path relative. I think there is a function to do that in Node path package.
We should also check whether Brunch provides backslash paths under Windows (app\foo\bar.js). If it is, should we normalize them into forward slash paths? Personally I think we should, since we'll use the path in browsers. (We can use sindresorhus/slash to convert them.)

I'll have a further look ASAP, but if you can investigate these things before me it would be great 🙂

cxw42 · 2018-09-09T15:18:39Z

Thanks for the quick response!

Edited Tested on Win8.1 using Node 8.11.4 x86, npm 6.4.1, npx 10.2.0 (PATHedited per npx issue 144 comment), brunch 2.10.17. Tested in both cmd and Cygwin x64.

When I run brunch, I do get relative paths from file.path, e.g., vendor/foo.js, app/page1.js.
I also do get forward slashes in file.path, even though path.sep is \\.
- However, I also see that .../bin/brunch uses path.join(). Would you like the code to include sindresorhus/slash just in case, and apply it if path.sep==='\\'?

tkesgar · 2018-09-10T14:09:48Z

I think adding sindresorhus/slash would be nice for future-proofing (just in case Brunch changes its API), but for now this should be enough.

I'll merge and publish this ASAP, thank you for your help!

cxw42 · 2018-09-10T14:21:30Z

@tkesgar Thanks! Let me know if you ever run across any slash-related problems, and I'll do likewise.

cxw42 pushed a commit to cxw42/brunch-test that referenced this pull request Sep 8, 2018

Add nested module in lib, and __filename

9f680f2

- `lib/simpletest` now requires `./lib/inner` - Replace `__filename` with the filename using tkesgar/replacer-brunch#1

cxw42 commented Sep 8, 2018

View reviewed changes

tkesgar merged commit 5424244 into tkesgar:master Sep 10, 2018

cxw42 mentioned this pull request Sep 21, 2018

Take over ocombe/assetsmanager-brunch#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide path to replace(); undefined value -> empty replacement string #1

Provide path to replace(); undefined value -> empty replacement string #1

cxw42 commented Sep 8, 2018

cxw42 Sep 8, 2018

cxw42 Sep 8, 2018

tkesgar commented Sep 9, 2018

cxw42 commented Sep 9, 2018 •

edited

Loading

tkesgar commented Sep 10, 2018

cxw42 commented Sep 10, 2018

Provide path to replace(); undefined value -> empty replacement string #1

Provide path to replace(); undefined value -> empty replacement string #1

Conversation

cxw42 commented Sep 8, 2018

cxw42 Sep 8, 2018

Choose a reason for hiding this comment

cxw42 Sep 8, 2018

Choose a reason for hiding this comment

tkesgar commented Sep 9, 2018

cxw42 commented Sep 9, 2018 • edited Loading

tkesgar commented Sep 10, 2018

cxw42 commented Sep 10, 2018

cxw42 commented Sep 9, 2018 •

edited

Loading