fix(bundler): Revisions are inserted into platform.index for all bundles #935

fragsalat · 2018-10-11T14:26:14Z

This fixes #933 by first writing all bundles to the disk and after wards update the file name in the platform.index for each bundle. This allows users to load all bundles in parallel via script tags.

PS: There were 18 tests failing even before I started^^

3cp · 2018-10-11T20:24:50Z

Are you running nodejs v10? The test doesn't work on v10 because of lack of mock-fs support.

3cp · 2018-10-11T20:26:21Z

@JeroenVinke do you know why this PR didn't trigger circleci check? There should be 3 checks.

fragsalat · 2018-10-16T13:39:03Z

@huochunpeng @JeroenVinke ping

3cp · 2018-10-16T20:50:41Z

@fragsalat please check the code comments.

fragsalat · 2018-10-17T08:49:38Z

I'm actually not seeing any. Did you may forgot to submit the review?

3cp · 2018-10-11T21:05:28Z

lib/build/bundle.js

-    //Replace the reference to the vendor bundle in the "index.html" file to use the correct build revision file (or remove the build revision hash);
-    const createSrcFileRegex = require('./utils').createSrcFileRegex;
-    let indexLocation = platform.index;
+  async writeRevToIndex(platform) {


Cannot use async/await syntax because it's only supported in nodejs v10.1.0+.
We need to support nodejs v8.9.0+.

Hmmm mdn tells me that the support in nodejs starts of 7.6.0
https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Operators/await

I tested it with 8.9.0 and for me it's working

I don't remember where I got v10.1.0+. Obviously false statement.

No problem :)

3cp · 2018-10-11T21:09:22Z

lib/build/bundler.js

@@ -232,7 +232,12 @@ exports.Bundler = class {
  }

  write() {
-    return Promise.all(this.bundles.map(x => x.write(this.project.build.targets[0])));
+    return Promise.all(this.bundles.map(bundle => bundle.write(this.project.build.targets[0])))


Why not call the rev writing inside bundle.js write(), like the current implementation?

You can use sync write to avoid promise.

It was actually like this except that the write was asynchronous. Once the vendor bundle was reached it wrote the revisions to the html but the vendor bundle doesn't know about the revisions of other files. I don't wanted to introduce code to pass revisions of other bundles into the last bundle because it felt a bit dirty and therefore I delayed the write process.

Regarding the sync write: I started with that solution but it were a bit hard because you use a mkdir function in the async write which is based on callbacks. I don't wanted to create a sync write function without this mkdir stuff as it would mean different behavior for those functions.

3cp · 2018-10-17T09:34:34Z

My bad

3cp · 2018-10-17T09:39:56Z

The reason your PR didn't trigger circleci check is that your commit is on very old master (before circleci was turned on), please rebase to latest master.

fragsalat · 2018-10-17T09:54:57Z

I pulled aurelia/cli@master into my branch but circle ci still don't trigger

3cp · 2018-10-17T10:49:22Z

lib/build/bundler.js

+    return Promise.all(this.bundles.map(bundle => bundle.write(this.project.build.targets[0])))
+      .then(async() => {
+        for (let i = this.bundles.length; i--; ) {
+          await this.bundles[i].writeRevToIndex(this.project.build.targets[0]);


Just realised you did serial write on index file. 👍
I am not familiar with async/await, took me quite a while to understand what's doing here.

3cp · 2018-10-17T11:00:56Z

👍 looks good, and worked for me.
I didn't use async/await, the serialised await spun my head, but it's a cool usage.

3cp · 2018-10-17T11:09:24Z

lib/build/bundle.js

-      } else {
-        logger.error(err);
+      // Validate rev is enabled and a hash was generated
+      if (platform.index && this.hash) {


Found a bug, you need to write empty hash to index file.
Consider:

au run --env prod # terminate it, then au run

The second au run need to overwrite index file to empty hash.

Oh my gosh. Thanks for pointing that out. The check for the hash was to much^^

3cp · 2018-10-17T21:06:05Z

lib/build/bundle.js

+      if (configMatcher.test(indexFile)) {
+        indexFile = indexFile.replace(configMatcher, 'src="$2' + bundleLocation + '"');
+        await fs.writeFile(platform.index, indexFile);
+        logger.info(`INFO [Bundle] Updated file name for ${this.moduleId} in ${platform.index}`);


Please remove "INFO [Bundle] " prefix, logger.info does that for you.

You got duplicated prefix.

INFO [Bundle] Writing app-bundle.js... INFO [Bundle] Writing vendor-bundle.js... INFO [Bundle] INFO [Bundle] Updated file name for vendor-bundle in index.html INFO [Bundle] INFO [Bundle] Updated file name for app-bundle in index.html Finished 'writeBundles'

3cp · 2018-10-17T21:06:21Z

lib/build/bundle.js

      }
+    } catch (error) {
+      logger.error(`ERROR [Bundle] Couldn't update file name with revision in ${platform.index}`, error);


Please remove "ERRPR [Bundle] " prefix, logger.error does that for you.

fragsalat · 2018-10-18T08:52:24Z

Fixed the logger stuff :)

3cp · 2018-10-18T08:56:03Z

@JeroenVinke could you review this? Looks good to me, not critical feature but does not hurt.

JeroenVinke · 2018-10-20T06:51:07Z

Hey @fragsalat and @huochunpeng :)

I've had a look, and had to think about the for loop for a while too 😄

Only thing I could find to complain about is the name of the function writeRevToIndex. I believe this function also updates the bundle names in index.html when rev isn't enabled. If you agree, would you mind changing the name? Then we can merge this :)

Great work @fragsalat

Alexander-Taran · 2018-10-22T09:10:27Z

@JeroenVinke naming is fixed (-:

fragsalat · 2018-11-14T12:07:19Z

Still not merged...?

JeroenVinke · 2018-11-14T18:54:39Z

Sorry about that, thanks a lot for your efforts @fragsalat

fix(bundler): Revisions are inserted into platform.index for all bundles

435557c

3cp reviewed Oct 17, 2018

View reviewed changes

Merge branch 'master' of github.com:aurelia/cli

bb8d1cd

3cp reviewed Oct 17, 2018

View reviewed changes

Fixed missing replacing of bundle paths without revision

57015e5

fragsalat force-pushed the master branch from 7ae320f to 57015e5 Compare October 17, 2018 11:33

3cp reviewed Oct 17, 2018

View reviewed changes

Fixed wrong logger usage

ccf1014

Fixed file name

be2e4e7

JeroenVinke merged commit e193d57 into aurelia:master Nov 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bundler): Revisions are inserted into platform.index for all bundles #935

fix(bundler): Revisions are inserted into platform.index for all bundles #935

fragsalat commented Oct 11, 2018 •

edited

Loading

3cp commented Oct 11, 2018 •

edited

Loading

3cp commented Oct 11, 2018

fragsalat commented Oct 16, 2018

3cp commented Oct 16, 2018

fragsalat commented Oct 17, 2018

3cp Oct 11, 2018

fragsalat Oct 17, 2018

fragsalat Oct 17, 2018

3cp Oct 17, 2018

fragsalat Oct 17, 2018

3cp Oct 11, 2018

3cp Oct 11, 2018

fragsalat Oct 17, 2018

3cp commented Oct 17, 2018

3cp commented Oct 17, 2018

fragsalat commented Oct 17, 2018

3cp Oct 17, 2018

3cp commented Oct 17, 2018

3cp Oct 17, 2018

fragsalat Oct 17, 2018

fragsalat Oct 17, 2018

3cp Oct 17, 2018

3cp Oct 17, 2018

3cp Oct 17, 2018 •

edited

Loading

fragsalat commented Oct 18, 2018

3cp commented Oct 18, 2018

JeroenVinke commented Oct 20, 2018

Alexander-Taran commented Oct 22, 2018

fragsalat commented Nov 14, 2018

JeroenVinke commented Nov 14, 2018

fix(bundler): Revisions are inserted into platform.index for all bundles #935

fix(bundler): Revisions are inserted into platform.index for all bundles #935

Conversation

fragsalat commented Oct 11, 2018 • edited Loading

3cp commented Oct 11, 2018 • edited Loading

3cp commented Oct 11, 2018

fragsalat commented Oct 16, 2018

3cp commented Oct 16, 2018

fragsalat commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3cp commented Oct 17, 2018

3cp commented Oct 17, 2018

fragsalat commented Oct 17, 2018

Choose a reason for hiding this comment

3cp commented Oct 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3cp Oct 17, 2018 • edited Loading

Choose a reason for hiding this comment

fragsalat commented Oct 18, 2018

3cp commented Oct 18, 2018

JeroenVinke commented Oct 20, 2018

Alexander-Taran commented Oct 22, 2018

fragsalat commented Nov 14, 2018

JeroenVinke commented Nov 14, 2018

fragsalat commented Oct 11, 2018 •

edited

Loading

3cp commented Oct 11, 2018 •

edited

Loading

3cp Oct 17, 2018 •

edited

Loading