Closed tommyZZM closed 7 years ago
Please fix space issues around ===
and {}
, etc. to adhere to style.
Ideally remove white-space only changes, i.e. from Readme.
Also since you added sections to Readme please update the Table of Contents via:
npm install -g doctoc
doctoc README.md
Overall LGTM assuming all current features still work and @bendrucker agrees.
:+1: to Thorsten's style comments. I'll help with the readme when the code's ready. I'd prefer a much more concise vanilla Node example there and a link to both Grunt and Gulp examples in the wiki.
// as the second arg is option from browserify api ,
// so i remove the method and make it simple
module.exports = function shim(file ,opts) {
var shimConfig = opts.shim || {};
}
browserify()
.add('./src/Main.js')
//opts arg comes from here, the option has a default object like { _flag:false }
.transform(browserifyShim,{
shim:{
react:"global:React"
}
})
Still some pretty significant style and docs issues to address. I also think your priority on options is backwards. Options should override the package.
i fix code style and merge these commits together~
Still a lot of whitespace that needs to be removed. Also I definitely want Gulp to be a wiki thing and not part of the readme.
restore README.md :blush:
Thanks for sticking to it. There are still some bugs I noticed that need to be addressed before we can write tests for this and address docs updates.
i think checking "browserify-shim" in package.json maybe unnessary at all :persevere:
@thlorenz What do you think about config handling here? I think this is an opportunity to satisfy all the people who've complained about not being able to require files above the package directory. If config is passed in via browserify options, the approach would be to run mothership and merge results if any are turned up, but not fail if none are found.
@bendrucker totally agree. I thought this PR solves at least part of this, but if it doesn't we should go for exactly what you are describing!
Any news about this PR merge ?
Remaining:
Really appreciate the effort you've invested thus far. After doing another review, it looks to me like there's still quite a lot to do here. In lieu of asking for what seems like hours of additional work to fix bugs and add tests, I'd like to propose that we continue to require a package.json config but then extend it with the transform options. If you wanted to omit config, you'd have to do:
{
"browserify-shim": {}
}
So this won't satisfy the people who want to require above their package dir, but it does help people looking to provide config via code/build tools rather than JSON.
Also please post a comment after you push commits, otherwise I don't get notified
Really looking forward for this to be completed and merged :) Could be useful in karma where I want to test a shim without having to shim the module itself.
I might pick this up at some point and try to finish it up since I'd like to do some significant stylistic/docs cleanup. In the meantime, anyone who needs it can continue to rely on the fork branch for a bit.
It's been a few months, any progress? It's a feature I am very much looking forward to.
No, you'd see commits if there were progress. This PR still has a ways to go before it's ready and doing that work isn't at the top of my priority list right now.
Thanks for your work on this. Since the code is not ready to merge and I don't intend to ship this feature myself, I'm closing due to inactivity.
add a Node.js API for user to provide browserify-shim config using node.js APIYou can also provide browserify-shim config using transform option. this will be useful when using browserify+gulp Such as example below. ``` javascript var gulp = require("gulp"); var gutil = require("gulp-util"); var source = require('vinyl-source-stream'); var browserify = require("browserify"); var babelify = require("babelify"); var browserifyShim = require("browserify-shim"); gulp.task("mytask",function(){ return browserify() .add('./src/Main.js') .transform(babelify) .transform(browserifyShim,{ shim:{ react:"global:React" } }) .bundle() .pipe(source('./main.js')) .pipe(gulp.dest('./dist/js/')); }); ```