Adding optional argument to append fingerprint hash with a new name #3

gcscoder · 2018-05-25T14:49:10Z

No description provided.

coderbydesign · 2018-05-29T12:57:50Z

index.js


  this.initializerDirectory = initializerDirectory;
  this.needsFingerprint = needsFingerprint;
+  this.fingerprintName = fingerprintName || 'ASSET_FINGERPRINT';


I'm wondering if it might be beneficial to have some sort of validation/constant format enforcement here to ensure something valid is written to the Ruby file.

I could add a check for names that have A-Z and _ in the middle and throw an error when it does not match.

On that note, do we want to lock them to something like this to enforce a convention, raise a meaningful error if not, and add something to the docs? Or leave off the "FINGERPRINT" part but still enforce the rest:

/^([A-Z]|_)*_FINGERPRINT$/

This would allow for something like "TEST_THIS_FINGERPRINT" as well.

Yeah, I think having that as part of the name is a good idea so that it is always clear.

coderbydesign · 2018-05-29T12:58:02Z

index.js

+        let defaultRun = this.fingerprintName === 'ASSET_FINGERPRINT';
+        let initializerPath = `${this.initializerDirectory}/asset_fingerprint.rb`;
+        const newline = defaultRun ? '' : '\r\n';
+        const fsMethod = defaultRun ? 'writeFileSync' : 'appendFileSync';


When you run this script more than once, it's going to append the custom constant multiple times. Is there a way we might be able to prevent that?

I could read the file prior to writing the value back out. If I find a string that matches the value for the name I could throw an exception.

Is that what we'd want to do though? Or just no-op? The way it is (at least how it worked for me locally, which was what I was curious about when we talked last week) is that every subsequent build will just append a duplicate of the constant, or if files have changed, a new value for the same constant, similar to this:

ASSET_FINGERPRINT='lkasd7fdjassdk23' CUSTOM_ASSET_FINGERPRINT='lkasd7fdjassdk23' CUSTOM_ASSET_FINGERPRINT='hhgdks553l2343n' CUSTOM_ASSET_FINGERPRINT='hhgdks553l2343n'

Ideally, the user should not call the plugin twice in the same build. I would not want multiple instances of the same constant with the same or different value.

But if it's appending, even on subsequent builds you would end up accumulating multiple instances of that appended constant, right? I was referring to if I run the build command once locally, then run it again a second time.

coderbydesign · 2018-05-29T12:58:11Z

index.js

+        const fsMethod = defaultRun ? 'writeFileSync' : 'appendFileSync';
+        let output = `${newline}${this.fingerprintName} = '${stats.hash}'`;
+        fs[fsMethod](initializerPath, output);
+        console.log(`asset-fingerprint-webpack-rails: updated file ${initializerPath} with ${this.fingerprintName} = ${stats.hash}`)


This doesn't reflect any secondary append to the file, does it?

It will continue to append unless default arguments, or the value for fingerprintName === 'ASSET_FINGERPRINT' are passed.

Sorry, I was just referring to the logging.

Yeah, it should log with every run.

coderbydesign · 2018-05-30T15:48:54Z

index.js

+
+  function _validateFingerprintName(fingerprintName) {
+    if(!/^([A-Z]|_)*_FINGERPRINT$/.test(fingerprintName)) {
+      throw new Error('Please supply a fingerprint name that is all caps separating words by underscores (i.e. CUSTOM_ASSET_FINGERPRINT).');


We may want to note something about the _FINGERPRINT being a part of the validation? Can we add something to the README about this requirement as well?

Yes, I'll add that.

coderbydesign · 2018-05-30T16:08:27Z

spec/indexSpec.js

+      });
+    });
+
+    describe("fingerprint name", function() {


Honestly just curious, but in the case where there are different setups for the same describe (like in this case where we have 3 "fingerprint name" blocks) is this an idiomatic pattern, vs having them nested under one block like below?

describe("fingerprint name", function(){ describe("valid", function(){ beforeEach(function() { ... }); it("should use value when provided", function() { ... }); }); describe("invalid", function(){ beforeEach(function() { ... }); it("should validate for duplicate name", function() { ... }); }); });

The return values setup for the spyOn calls for fs are different and must be setup in a new describe section. There is one test I can move up with the others that has the same criteria.

coderbydesign · 2018-05-30T16:50:19Z

index.js

+    _validateFingerprintName(fingerprintName);
+
+    if(_checkForExistingFingerprint(fingerprintName, this.initializerPath)) {
+      throw new Error('The provided fingerprint name has already been used.');


This is being thrown for me on secondary runs. I'm testing it by running two independent bundles. The first plugin is setup as a default: new AssetFingerprintPlugin('config/initializers', true) which writes ASSET_FINGERPRINT. The second build sets the plugin up with the optional argument: new AssetFingerprintPlugin('config/initializers', true, 'TEST_FINGERPRINT'). On the first run, it creates the file, adds the default and then the override as expected. But if I run that delegation script again it throws this error. Is there a more deterministic way to test/implement this that you're using?

Per our discussion, this was a local testing implementation issue. 👍

coderbydesign · 2018-05-31T10:46:06Z

@gcscoder - one last thing is that we should bump the package.json version. I'm guessing a minor bump should be fine since it should be backwards compatible?

coderbydesign

Looks good to me, thanks for this PR!

gcscoder · 2018-05-31T20:55:02Z

Glad I could help!

Adding optional argument to append fingerprint hash with a new name

8e615f6

coderbydesign reviewed May 29, 2018

View reviewed changes

Adding input validation checks for fingerprintName

ef931f1

coderbydesign reviewed May 30, 2018

View reviewed changes

Updated readme, version, adding note about fingerprint name format

345c376

coderbydesign approved these changes May 31, 2018

View reviewed changes

coderbydesign merged commit 0788866 into coderbydesign:master Jun 1, 2018

Adding optional argument to append fingerprint hash with a new name #3

Adding optional argument to append fingerprint hash with a new name #3

Uh oh!

Conversation

gcscoder commented May 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coderbydesign commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderbydesign left a comment

Choose a reason for hiding this comment

Uh oh!

gcscoder commented May 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderbydesign commented May 31, 2018 •

edited

Loading