aurelia / bundler

A library for bundling JavaScript, HTML and CSS for use with SystemJS.
MIT License
37 stars 25 forks source link

#143 - Added revisioning to html import template bundler #144

Closed fragsalat closed 8 years ago

fragsalat commented 8 years ago

Regarding the issue #143 I added the missing revisioning to html import template bundler. Therefore I changed the code to first generate the bundle, create the fileName, check for an existing bundle and then write the file. This order of tasks is required because the generated hash for the revisioning requires the content.

Adding the rev: true to the bundle options will now result in a bundle name like this view-bundle-cd82b12640

Example config:

'view-bundle': {
    'htmlimport': true,
    'includes': [
        'app/**/*.html'
    ],
    'options': {
        'rev': true
    }
}

Results in: view-bundle-cd82b12640.html

ahmedshuhel commented 8 years ago

Thanks for the PR @fragsalat . Can we add a few tests for this? And please sign the CLA.

fragsalat commented 8 years ago

I though I signed it already. The PR in aurelia/path was also merged.

ahmedshuhel commented 8 years ago

@EisenbergEffect Could you please check why our CLA service is complaining here?

EisenbergEffect commented 8 years ago

@fragsalat We switched over to a new mechanism for handling the CLA. For some reason a few of our contributors didn't import correctly so we may need you to re-sign. You can click the link in the CLA check below.

fragsalat commented 8 years ago

Done :+1: Thx for the note :)

EisenbergEffect commented 8 years ago

@fragsalat Our CLA Assistant isn't able to track you as signing the CLA. Were you able to sign it using the link in the details section of this issue?

fragsalat commented 8 years ago

@EisenbergEffect Yes I did. screenshot from 2016-10-04 09-47-56

CLAassistant commented 8 years ago

CLA assistant check
All committers have signed the CLA.

EisenbergEffect commented 8 years ago

@fragsalat Yes. I can confirm. I'm trying to figure out why the cla assistant doesn't think you have though. Do you have more than one Github account? or is the account you pushed this PR with not associated with the correct email address by any chance? @PWKad What was it that happened to you when the CLA assistant wasn't working? Wasn't it something like that?

EisenbergEffect commented 8 years ago

@ahmedshuhel I have manually confirmed that the CLA is signed so you can merge this if you think it's ready.

fragsalat commented 8 years ago

@EisenbergEffect After some research my guess is that the github account config for my work was used to create the commit. So the Email within the commit is different to the EMail of these account which I can't change since the github is a enterprise self hosted version. My suggestion is that I do the commit again with the correct email. Just give me some minutes

EisenbergEffect commented 8 years ago

Sounds good. Apologies for the complications!

fragsalat commented 8 years ago

Works yey

plwalters commented 8 years ago

That is correct the pr was created by your normal one but work account or machine created the commit itself so it trips up on matching.

ahmedshuhel commented 8 years ago

@fragsalat It would be great if you could add a few tests for this.

fragsalat commented 8 years ago

Until now there were no tests at all but I can have a look at setting up some :)

fragsalat commented 8 years ago

Sry didn't checkt the correct repo^^ I'll do it

ahmedshuhel commented 8 years ago

Awesome!

fragsalat commented 8 years ago

@ahmedshuhel I added 2 tests which are covering the revisioning of the file name based on the file content and the general bundling output.

Btw: wallaby seem to be a nice thing. Even if it's just to expensive for what it does.

ahmedshuhel commented 8 years ago

@fragsalat one test is failing on my local machine. Can you confirm?

  20 passing (654ms)
  1 failing

  1) A template bundler writes the bundle to disk:
     Error: Expected '' to be '<template id="tmp1">test</template>\n<template id="tmp2">test</template>'
      at assert (node_modules/expect/lib/assert.js:29:9)
      at Expectation.toBe (node_modules/expect/lib/Expectation.js:66:28)
      at Context.<anonymous> (test/template-bundle.spec.js:22:39)
fragsalat commented 8 years ago

How do you run it? I'm using the trial version of Wallaby.js integration for IntelliJ and my result is ok.

No failing tests, 21 passing

And here the console output.

wallaby.js started
core v1.0.300
Finished executing 21 affected test(s)
fragsalat commented 8 years ago

@ahmedshuhel Are you using the default config of wallaby with phantomjs or did you do any customization to the wallaby config?

Even after almoast trying it 20 times the error doesn't appear.

ahmedshuhel commented 8 years ago

@fragsalat Sorry, could not reply you earlier. I ran the test with npm test which uses mocha and I believe the configuration for wallaby.js is same. I used wallaby.js in the past but unfortunately it is not installed on my machine now.

fragsalat commented 8 years ago

@ahmedshuhel With mocha it fails as well. Thx for that info, I gonna fix the PR and mention you when it's done :)

fragsalat commented 8 years ago

@ahmedshuhel I fixed the unit tests and refactored the code a bit. The grouping makes a bit more sense and is also testable I think. Now the generation of output content and file name are in single functions for to be testable and the output writing went back to the main function. What do you think?

ahmedshuhel commented 8 years ago

@fragsalat Looks good to me. There's some room for improvement but we can get to that later. I am moving the code base to typescript gradually. Thank you for the contribution.

fragsalat commented 8 years ago

@ahmedshuhel Thank you :)

fragsalat commented 8 years ago

@ahmedshuhel When do you will release this? In the last cycle it wasn't published.

EisenbergEffect commented 8 years ago

@ahmedshuhel Please let me know if you are ready for a release so I can release it and prepare the blog post notes.

jp7677 commented 8 years ago

@ahmedshuhel, @EisenbergEffect Would love to see a new release too. Note that master is already far superior to the last 0.4 release from may. Next to this PR the current official version lacks source maps support and won't minify your html and css imports. The last one is quite a serious regression imho. I understand the focus on Aurelia CLI, but jspm and this bundler works actual pretty cool too once you get used to the working of systemjs.

Is there anything I could do to help?

ahmedshuhel commented 8 years ago

Sorry guys. We will make sure it gets out with the next batch. On Oct 29, 2016 9:01 PM, Jens Peters notifications@github.com wrote:@ahmedshuhel, @EisenbergEffect Would love to see a new release too. Note that master is already far superior to the last 0.4 release from may. Next to this PR the current official version lacks source maps support and won't minify your html and css imports. The last one is quite a serious regression imho. I understand the focus on Aurelia CLI, but jspm and this bundler works actual pretty cool too once you get used to the working of systemjs.

Is there anything I could do to help?

—You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or mute the thread.