rotundasoftware / cartero

Modular front end development for multi-page web applications.
MIT License
204 stars 25 forks source link

Emit browserifyInstanceCreated when creating parcelify #28

Closed ferlores closed 10 years ago

ferlores commented 10 years ago

We are using cartero in our project and we came across with the following use case: I want to create a common parcel (js, css and img set bundle) that can be shared across some of the parcels (taking advantage of browser cache).

To achieve that I created a new parcel called commonBundle, which will require the common modules. Now I need to (browserify.)exclude those files from all the parcels except commonBundle. With this change I can call cartero like this:

cartero.on('browserifyInstanceCreated', function(parcelName, browserifyInstance) {
    if (parcelName !== 'commonBundle') {
      browserifyInstance
        .exclude('jquery')
        .exclude('commonStuff');
    }
});
ferlores commented 10 years ago

BTW, if there is a better way to achieve this without this change I would love to hear it :)

dgbeck commented 10 years ago

Hi @ferlores ! Seems like a great approach to me. Have you verified this is working as you'd expect?

Another approach to separating out jquery or other common libraries is to use browserify-shim. You can even grab them a CDN or whatever and access them through their standard globals (e.g $).

ferlores commented 10 years ago

Thanks for the fast reponse :) Actually I still have a problem to get it working. I need to expose the modules that are required by commonBundle to the global scope, so the other bundle can find those modules. I've changed the code to:

cartero.on('browserifyInstanceCreated', function(parcelName, browserifyInstance) {
    if (parcelName !== 'commonBundle') {
      browserifyInstance
        .external('jquery')
        .external('commonStuff');
    }
});

Now I'm trying to make commonBundle (and jquery and commonStuff) available. I'm trying without success to use the standalone feature from browserify. Any suggestion?

Thanks F

ferlores commented 10 years ago

Ok, got it working with the following code:

 cartero.on('browserifyInstanceCreated', function(parcelName, browserifyInstance) {
    if (parcelName !== 'commonBundle') {
      browserifyInstance
        .external('jquery')
        .external('commonStuff');
    } else {
      browserifyInstance
        .require('jquery')
        .require(path.resolve(paths.custom_node_modules, 'commonStuff'), {expose: 'commonStuff'})
    }
  });

Now I'm just wondering if is not better that parcelify is the one that adds the parcel name to the triggering of browserifyInstanceCreated. What do you think?

dgbeck commented 10 years ago

Yeah I like that. In the parcelify context it would just be mainPath . On line 62 of parcelify's index.js, we'd have

_this.emit( 'browserifyInstanceCreated', browserifyInstance, this.mainPath );

Sound good?

ferlores commented 10 years ago

Yes, but I think that cartero should emit the parcel name instead of the mainPath. I've updated this PR and send this one to parcelify: https://github.com/rotundasoftware/parcelify/pull/13

dgbeck commented 10 years ago

Hi @ferlores ,

Sorry, I think I made a mistake here and we have to backtrack (kinda). Since parcelify only supports one entry point it doesn't make sense for it to emit that entry point with the browserifyInstanceCreated event. The caller should already know the entry point on which parcelify was invoked. After all, it was the caller that invoked it with that entry point!

I think your first approach is the way to go, but I'd like to stay away from introducing a new parcelName concept, and instead just emit the mainPath. browserify is just concerned with the mainPath, and this event is a browserify specific event, so coupling the two is quite fitting.

If you can update the PR so that cartero just emits the mainPath after the corresponding browserify instance I'll get it merged and bumped asap.

Thanks for your feedback and for suggesting this great idea!

ferlores commented 10 years ago

Oh I overlooked the fact the parcelify works on one entry point at a time. I agree, the previous approach is better.

Regarding the parcelName, I think that the concept is already there. For instance in the metadata.json you are mapping the absolute path of the parcel to the new asset directory.

To emit just the parcelName(xxx) is not the best but emitting the path to js entry file(/path/to/xxx/index.js) is not either. Maybe the best is to emit the same path that you write in the metadata.json(/path/to/xxx). What do you think?

Anyway I'm happy updating my PR with whatever we decide here :) On Oct 2, 2014 10:21 AM, "David Beck" notifications@github.com wrote:

Hi @ferlores https://github.com/ferlores ,

Sorry, I think I made a mistake here and we have to backtrack (kinda). Since parcelify only supports one entry point it doesn't make sense for it to emit that entry point with the browserifyInstanceCreated event. The caller should already know the entry point on which parcelify was invoked. After all, it was the caller that invoked it with that entry point!

I think your first approach is the way to go, but I'd like to stay away from introducing a new parcelName concept, and instead just emit the mainPath. browserify is just concerned with the mainPath, and this event is a browserify specific event, so coupling the two is quite fitting.

If you can update the PR so that cartero just emits the mainPath after the corresponding browserify instance I'll get it merged and bumped asap.

Thanks for your feedback and for suggesting this great idea!

— Reply to this email directly or view it on GitHub https://github.com/rotundasoftware/cartero/pull/28#issuecomment-57664899 .

dgbeck commented 10 years ago

I think best to keep it as mainPath. At the time this event is emitted the parcel object has not even been created. Feels like a lower level event to me, best to keep it all on that level.

ferlores commented 10 years ago

@dgbeck I've just updated the PR emitting only mainPath. Once it gets merged, do you think you will be able to release a minor fix to NPM? Thanks!

dgbeck commented 10 years ago

Done at v3.1.0! Thanks again!