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

Grunt-Browserify v2 #109

Closed
2 of 4 tasks
jmreidy opened this issue Nov 22, 2013 · 32 comments
Closed
2 of 4 tasks

Grunt-Browserify v2 #109

jmreidy opened this issue Nov 22, 2013 · 32 comments

Comments

@jmreidy
Copy link
Owner

jmreidy commented Nov 22, 2013

Pulling in @bclinkinbeard, @hurrymaplelad, and @shama.

It's time to talk about v2. I've been keeping this on the back burner for awhile, but I think I finally have bandwidth to work on it.

What needs to get done:

  • a rewrite of the core task. It's a huge function with a bunch of if/elses. It needs to be broken down into a much more logical structure. I don't necessarily think we need to expose the functionality via module.exports and unit test it or anything, but cleanliness will definitely help with maintainability.
  • Integrate watchify
  • Normalize how paths are handled. Right now, it's a bit haphazard and definitely non-standard, mostly as a result of filepath logic being changed on the fly. I'd like to follow grunt-watchify's example here, and just normalize to path.resolve.
  • One huge thing for me - changing how alias and external are handled. It's incredibly annoying to have to specify aliases for a common module, and specify the same aliases AGAIN as external for another module. I'd like to declare common module aliases as an array, and then re-use that array to pass to external of another g-b task. (If this doesn't make sense, I can put together a sample gist.)

What else?

@hurrymaplelad
Copy link

@jmreidy thanks for getting this rolling. How 'bout some development time speedups, like:

  1. Share the browserify cache across bundles within a single run of grunt
  2. Share the browserify cache across multiple grunt runs

@jmreidy
Copy link
Owner Author

jmreidy commented Nov 22, 2013

@hurrymaplelad Good ideas. I made an attempt on sharing the browserify cache across bundles awhile ago, and ran into some odd behavior - but that was before watchify even existed, so I'm game to trying again.

@RoboTeddy
Copy link
Contributor

👍 (unfortunately I don't have the bandwidth to contribute, just cheering you guys on!)

@terinjokes
Copy link

I have a fork of this module that works alone with epeli/browserify-externalize, that is kinda a kludge due to this module being a huge function full of if functions. Also might be a useful feature push upstream.

@jmreidy
Copy link
Owner Author

jmreidy commented Feb 4, 2014

There's a working beta version of v2 on NPM, which can be installed by specifying '2.0' in package.json. Currently it works exactly the same as v1.x, but with an internal rewrite and watchify support. Use option watch:true to trigger watchify; if you're not using another grunt task to keep the process alive, pass keepAlive: true as well.

@mikefrey
Copy link

Too late to ask for exorcist support in v2? https://github.com/thlorenz/exorcist

@svnlto
Copy link

svnlto commented Feb 13, 2014

Too late to ask for exorcist support in v2? https://github.com/thlorenz/exorcist

+10

@jmreidy
Copy link
Owner Author

jmreidy commented Feb 13, 2014

Doesn't this work already just by supplying a transform?

@mikefrey
Copy link

@jmreidy Ha! Yeah... I suppose it would. Whoops.

@mikefrey
Copy link

Not to hijack this issue with Exorcist stuff, but, using it as a transform doesn't work since it's a post bundle op (unless i'm missing something, which is possible). So, I ended up writing this: https://github.com/mikefrey/grunt-exorcise

If you feel like pulling it into grunt-browserify, by all means do so. Otherwise I'll just maintain it independently. Big thanks for a great module!

@jmreidy
Copy link
Owner Author

jmreidy commented Feb 17, 2014

@mikefrey Did you try using the postBundleCB option? https://github.com/jmreidy/grunt-browserify#postbundlecb

@mikefrey
Copy link

@jmreidy Yes, but I felt that given the amount of configuration that could go into it, and the stream-y nature of Exorcist, it would be better served as it's own grunt task that can be shared.

@jmreidy
Copy link
Owner Author

jmreidy commented Feb 20, 2014

I've been thinking a good amount about grunt-browserify and its future, and I think it needs to be simplified. A lot.

What does a simplified grunt-browserify look like?

  1. Remove the aliasMappings code. This was contributed by @bclinkinbeard and I know it's been used by a lot of people, but I've always struggled to understand its use cases, and it adds a lot of code and conceptual baggage.

  2. Remove the browserify-shim integration. @thlorenz has rewritten browserify-shim to work as a transform, and the existing b-shim integration isn't even compatible with the current browserify-shim version. Using b-shim as a transform should just work for folks, obviating the need for complexity in the grunt-browserify source.

  3. Make aliasing and externalizing easier. This is my biggest complain with grunt-browserify as it currently exists. I want to be able to easily have one bundle point at another and "know" what to externalize.

  4. Stick close to the browserify API. Browserify is improving quite rapidly of late, and I'd like grunt-browserify to stick as close to the browserify improvements as possible.

So that's where things are going - and where they will be shortly!

@terinjokes
Copy link

@jmreidy Externalizing might be easier in the future with native browserify, with the factor-bundle plugin.

@jmreidy
Copy link
Owner Author

jmreidy commented Feb 20, 2014

@terinjokes sweet. That looks awesome. Having grunt-browserify focus on arranging browserify file lists, plugins, and transforms seems like the right direction.

@joeybaker
Copy link
Contributor

After playing with grunt-watchify recently, i found that I needed the aliasMapping part of grunt-browserify, so I copy-pasted as a temporary solution. It probably makes a lot of sense as a transform/plugin for browserify?

/cc @bclinkinbeard

@bclinkinbeard
Copy link
Contributor

I think it needs to be simplified. A lot.

👍

[aliasMapping] probably makes a lot of sense as a transform/plugin for browserify?

That actually sounds like a great idea, and it could probably be assembled almost entirely from existing modules.

@terinjokes
Copy link

Can't aliasMappings just be done as a resolve function?

@joeybaker
Copy link
Contributor

I created a quick module to do alias mappings: https://github.com/joeybaker/remapify

@terinjokes yes, it probably could be a resolve function, but I find having a bit of structure around the process nice. IMO.

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 17, 2014

So I bit the bullet and rewrote the task from scratch. A key thing for me was to make the tests more intuitive; the need to create fixture files and compare Grunt output to the fixtures felt brittle, and certainly added a good deal of complexity. The new code has extrapolated the grunt task into its own runner, with grunt functionality injected into it. That means that the test code can properly exercise the runner's API without all of the fixture nonsense.

The runner itself has been dramatically simplified in favor of sticking as close to browserify as possible. When I originally wrote this task, the rich ecosystem of browserify transforms didn't yet exist, and plugins weren't even a feature. Now that there's so many browserify-based modules out there, I don't think a grunt task should bother reinventing the wheel.

Take a look at where things are at with v2New branch. I'd like to merge this to master in the next week or so, but would like the tires kicked pretty extensively before doing so.

@thlorenz
Copy link

I like the simplified approach, however I think you could take it one step further.

Everything I can specify in package.json should be configured there. That includes aliases via the browser field and transforms via the transform field.
Exposing another API to do the same (which IMO is not any simpler) increases the learning curve, as now it's confusing what browserify and some transforms document and what this library documents.

So to keep it simple it'd be better to stick to bare browserify APIs where possible and only reinvent APIs when that results in a significant gain.

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 17, 2014

@thlorenz I think, in general, that there's a browserify way of doing things, and a grunt way of doing things. As a grunt task that's using browserify, this project has to walk the line of both conventions.

I completely agree with you that the browserify approach has focused on using package.json to specify options. But that approach is atypical in grunt-land. Since this task is meant for grunt, I'm going to keep configuration in the Gruntfile.

That said, if there's a lot of confusion down the road, I'd be more than happy to change this approach; the current rewrite will make changes much, much easier.

@thlorenz
Copy link

@jmreidy understood.
In that case explaining what grunt-browserify config feature maps to which browserify config feature would help a lot to avoid confusion.

Maybe even a section in the documentation (or a blog post) that shows both ways to configure things side by side for an example project.

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 17, 2014

@thlorenz I think that's a really good idea.

@victorwpbastos
Copy link

@terinjokes the remapify lib can be used with transform?

@givankin
Copy link

One thing that's good in "grunt" way compared to "browserify" package.json-based way is that one can has any number of completely independent configurations.

@SerjoPepper
Copy link

I use browserify as client-js build tool, but I don't have "node_modules" directory for client code, so all dependencies use relative paths:
var ui = require('../lib/ui'); // instead of beauty require('lib/ui')
But it is inflexibly, when I rename folders or move files.
Aliasify and Remapify are good decisions, but have one disadvantage - aliased files will be included in result build forever, even if I don't require its in my code.
I found one uglify decision: before grunt-browserify task, I launch grunt-copy task, which copy my root directory to new 'node_modules' directory, so all modules paths will be resolved good.
I have suggestion - include this mechanic as 'baseDir' or 'rootDir' option. What do you think about it?

P.S. Sorry for bad English.

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 19, 2014

@SerjoPepper While that approach can definitely lead to cleaner requires statements, it goes against the browserify/node way of doing things, so I'm reluctant to add it as a capability to grunt-browserify.

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 19, 2014

Has anyone else hit on v2New yet? Any thoughts on its merge-ability?

@spencerbeggs
Copy link

Hey, @jmreidy. I just tested my application on v2New using b-shim and hbsfy. Everything works like a charm so far.

@victorwpbastos
Copy link

@jmreidy, i'm using the v2New branch. It's ok! When this branch will be freezed so i can use without any fear of code breaking?

@jmreidy
Copy link
Owner Author

jmreidy commented Mar 21, 2014

version 2 is merged!

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

No branches or pull requests