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.

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

Passes full file path to the replacement function #76

Closed
wants to merge 7 commits into from

Conversation

asins
Copy link

@asins asins commented Oct 14, 2016

#67 finishing version

index.js Outdated

doReplace();
}
});
};
Copy link
Owner

Choose a reason for hiding this comment

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

Please do not change spaces to tabs in your PR, it makes it hard to review. Can you change it back and push a new commit?

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "gulp-replace",
"version": "0.5.4",
"version": "0.5.5",
Copy link
Owner

Choose a reason for hiding this comment

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

This is a new feature, so we should bump to 0.6.0.

@asins
Copy link
Author

asins commented Oct 17, 2016

Resolved after performing gulp.dest() method to modify this pointer is a problem.

@lazd
Copy link
Owner

lazd commented Oct 17, 2016

@asins I'm not sure what you mean by:

gulp.dest() method to modify this pointer is a problem.

Can you elaborate?

@asins
Copy link
Author

asins commented Oct 18, 2016

demo:

npm install gulp gulp-replace --save-dev

add code for gulp-replace/index.js:

        if (options.passFileName && typeof replacement === 'function') {
          var userReplacement = replacement;
          replacement = function () {
              var context = {
                  filePath: file.path
              };
              return userReplacement.apply(context, arguments);
          };
        }

in demo folid tree -L 1

├── gulpfile.js
├── node_modules
├── package.json
└── src

The gulpfile.js:

const Fs = require('fs');
const Path = require('path');
const gulp = require('gulp');
const replace = require('gulp-replace');

var i = 0;
gulp.task('tests', function () {
    return gulp.src('./src/css/*.css')
        // .pipe(less())
        .pipe(replace(/url\((["']?)([^\1\)]+)\1\)/g, cssAssets, {passFileName: true}))
        .pipe(gulp.dest('./build/css'))
        // .pipe(cssnano())
        .pipe(gulp.dest('./dist/css'))

    function cssAssets($0, $1, url){
        console.log('------------>', i++, this.filePath, url);
        return $0;
    }
});

The css files:

/* src/css/a.css */
@font-face {font-family: "iconfont";
  src: url('../fonts/iconfont.eot');
  src: url('../fonts/iconfont.eot?#iefix') format('embedded-opentype'),
  url('../fonts/iconfont.woff') format('woff'),
  url('../fonts/iconfont.ttf') format('truetype'),
  url('../fonts/iconfont.svg#iconfont') format('svg');
}
.YT-has-passport{
    background: url("../images/alipayToYT.jpg") no-repeat center bottom #0496d1;
    background-size:100%;
}

/*src/css/b.css*/
@font-face {font-family: "iconfonts";
    src: url('../fonts/iconfont.eot');
    src: url('../fonts/iconfont.eot?#iefix') format('embedded-opentype'),
    url('../fonts/iconfont.woff') format('woff'),
    url('../fonts/iconfont.ttf') format('truetype'),
    url('../fonts/iconfont.svg#iconfont') format('svg');
}
@font-face{font-family: "asins-font";
    src: url('../fonts/asins.eot');
    src: url('../fonts/asins.eot?#iefix') format('embedded-opentype'),
    url('../fonts/asins.woff') format('woff'),
    url('../fonts/asins.ttf') format('truetype'),
    url('../fonts/asins.svg#iconfont') format('svg');
}
.icon-logo{margin: 40px auto; background:#e6e6e6 url(../images/icon-risk.png) 50% 50% no-repeat;}
.nc-container{ background: url(../images/loading.gif) 50% 50% no-repeat; height: 40px;}

run

Now, run gulp tests, you can see after gulp.dest() output after the first file this.filePath was changed.

~/gulptest » gulp tests
[09:40:49] Using gulpfile ~/gulptest/gulpfile.js
[09:40:49] Starting 'tests'...
------------> 0 /Users/asins/gulptest/src/css/a.css ../fonts/iconfont.eot
------------> 1 /Users/asins/gulptest/src/css/a.css ../fonts/iconfont.eot?#iefix
------------> 2 /Users/asins/gulptest/src/css/a.css ../fonts/iconfont.woff
------------> 3 /Users/asins/gulptest/src/css/a.css ../fonts/iconfont.ttf
------------> 4 /Users/asins/gulptest/src/css/a.css ../fonts/iconfont.svg#iconfont
------------> 5 /Users/asins/gulptest/src/css/a.css ../images/alipayToYT.jpg
------------> 6 /Users/asins/gulptest/build/css/a.css ../fonts/iconfont.eot
------------> 7 /Users/asins/gulptest/build/css/a.css ../fonts/iconfont.eot?#iefix
------------> 8 /Users/asins/gulptest/build/css/a.css ../fonts/iconfont.woff
------------> 9 /Users/asins/gulptest/build/css/a.css ../fonts/iconfont.ttf
------------> 10 /Users/asins/gulptest/build/css/a.css ../fonts/iconfont.svg#iconfont
------------> 11 /Users/asins/gulptest/build/css/a.css ../fonts/alinc.eot
------------> 12 /Users/asins/gulptest/build/css/a.css ../fonts/alinc.eot?#iefix
------------> 13 /Users/asins/gulptest/build/css/a.css ../fonts/alinc.woff
------------> 14 /Users/asins/gulptest/build/css/a.css ../fonts/alinc.ttf
------------> 15 /Users/asins/gulptest/build/css/a.css ../fonts/alinc.svg#iconfont
------------> 16 /Users/asins/gulptest/build/css/a.css ../images/icon-risk.png
------------> 17 /Users/asins/gulptest/build/css/a.css ../images/loading.gif
[09:40:49] Finished 'tests' after 31 ms

demo.zip

@asins
Copy link
Author

asins commented Mar 27, 2017

I found that filepath was the first time the file was sent to multiple files, because the function was rewritten several times, and the internal value was not changed.

index.js Outdated
filePath: file.path
};
userReplacement.apply(context, arguments);
};
Copy link
Owner

Choose a reason for hiding this comment

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

This is definitely a clever way to achieve this, but I wonder if users will ever use .bind() on their replacement functions and therefore be unable to use passFilePath.

index.js Outdated
};
return userReplacement.apply(context, arguments);
};
})(file.path);
Copy link
Owner

Choose a reason for hiding this comment

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

Got it, I see why you did this.

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "gulp-replace",
"version": "0.5.4",
"version": "0.6.2",
Copy link
Owner

Choose a reason for hiding this comment

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

Should be 0.6.0 but don't worry about changing it here, I'll change it when I release.

@@ -69,11 +84,17 @@ An optional third argument, `options`, can be passed.
Type: `Object`

##### options.skipBinary
Type: `boolean`
Type: `boolean`
Copy link
Owner

Choose a reason for hiding this comment

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

These actually need two spaces or a line break after them, otherwise they appear on the same line

Type: `boolean`
Default: `false`

Passes full file path to the replacement function
Copy link
Owner

Choose a reason for hiding this comment

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

Makes the full file path available inside the replacement function as this.filePath

gulp.task('templates', function(){
gulp.src(['file.txt'])
.pipe(replace(/foo(.{3})/g, function($0, str){
return str + 'foo' + this.filePath;
Copy link
Owner

Choose a reason for hiding this comment

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

return 'bar in' + this.filePath;

It is customary to replace foo with bar :) In this case, we can just tack the path onto the end.

"name": "gulp-replace",
"version": "0.5.4",
"name": "gulp-replace-fix",
"version": "0.6.4",
Copy link
Owner

Choose a reason for hiding this comment

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

Remove the package name change and the version increment please.

// };
// return userReplacement.apply(context, arguments);
// };
// })(file.path);
Copy link
Owner

Choose a reason for hiding this comment

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

Remove commented out code please.

@lazd
Copy link
Owner

lazd commented Mar 27, 2017

A couple notes:

  1. We should be consistent in referring to this as the filePath as is contains all of the path information, not just the name. Vinyl uses file.path, so perhaps it should be this.path.

  2. Perhaps we should just make the Vinyl file object the context with replacement = userReplacement.bind(file);. Then all of the instance methods and properties of the Vinyl file object will be available, not just the path. That would also result in 1. above being satisfied.

Thoughts?

@lazd
Copy link
Owner

lazd commented Mar 27, 2017

I found that filepath was the first time the file was sent to multiple files, because the function was rewritten several times, and the internal value was not changed.

Please provide tests in the tests/ directory with a single file and multiple files. If you wrote the tests before the code, you wouldn't have had this bug 😛 🤓

@lazd
Copy link
Owner

lazd commented Jun 20, 2017

I went ahead and used the approach in #80 for this. Thanks for your contribution; though it wasn't merged, it still helped define the API and the project is better for it.

@lazd lazd closed this Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants