Fixes #1335 Added parameter to build command to set custom filename #1741

birtony · 2019-10-27T21:54:41Z

Fixes #1335
Implemented logic for the custom filename option parameter --filename for the build command.

src/cmd/build.js

coveralls · 2019-10-28T21:28:16Z

Coverage remained the same at 100.0% when pulling c40cc67 on birtony:issue#1335 into 685e466 on mozilla:master.

rpl

Hi @birtony! follows a more complete round of review comments.

Let me know if you have any doubts, questions or if you get stuck on any of the following request for changes.

rpl · 2019-10-30T12:55:23Z

package-lock.json

@@ -4415,7 +4415,8 @@
      "requires": {
        "import-fresh": "^2.0.0",
        "is-directory": "^0.3.1",
-        "js-yaml": "^3.13.1"
+        "js-yaml": "^3.13.1",
+        "parse-json": "^4.0.0"


uhm... it doesn't seem that this PR includes any changes to the dependencies part of the package.json, and so there shouldn't be any change to package-lock.json, and so I'm wondering how this ended up into the PR.
have you installed the dependency using npm install instead of npm ci? using which version of npm?

Anyway, besides my curiosity about "how it did happen", we should remove this change from this PR.

My bad, @rpl. I have used npm install, version 6.11.3. I will remove the change in my next commit!

rpl · 2019-10-30T13:01:09Z

src/cmd/build.js

-    });
+  let packageName;
+  if (filename) {
+    packageName = `${filename}.zip`;


I was expecting that filename would be the full filename including the extension, at least if the filename we got has already an extension like ".zip" or ".xpi" (and this would also have the nice side-effect of making web-ext build able to create files already using the .xpi extension).

I'm also thinking that we should likely validate a bit the filename we got.

e.g. we may at least check that it is a simple file name and not a relative or absolute path, so that the file will still be stored in the artifacts dir (as the build command currently does).

From a quick look to the nodejs API docs, it seems to me that nodejs path.parse method may be useful to achieve that.

Okay, I see. I will work on implementing this.

Hey @rpl, do you think something like this would be sufficient?

if (filename) { try { const valFilename = path.parse(filename); if (valFilename.dir !== '') { throw new UsageError( 'Filename cannot contain any path' ); } else { packageName = `${filename}`; } } catch (error) { throw new UsageError( `Error validating the filename: ${error}`); } } else { // set default filename // ... }

Should we also limit the user on the number of extension types he/she can give to the filename?

Should we also limit the user on the number of extension types he/she can give to the filename?

You mean to only allow some kind of extensions (like .zip and .xpi) to be used as the extension in the final filename, right?
yeah that sounds reasonable to me.

Hey @rpl, do you think something like this would be sufficient?

I was thinking that we may first replace the part to interpolate and then do something like:

export function getPackageNameFromTemplate( filenameTemplate: string, manifestData: ExtensionManifest ): string { ... const packageName = ...; // Validate the resulting packageName string, after interpolating the manifest property // specified in the template string. const parsed = path.parse(packageName); if (parsed.dir) { throw new UsageError( `Invalid filename template "${filenameTemplate}". ` + `Filename "${packageName}" should not contain a path` ); } if (!['.zip', '.xpi'].includes(parsed.ext)) { throw new UsageError( `Invalid filename template "${filenameTemplate}". ` + `Filename "${packageName}" should have a zip or xpi extension` ); } return packageName;

rpl · 2019-10-30T13:22:26Z

src/program.js

@@ -381,6 +381,15 @@ Example: $0 --help run.
      default: true,
      type: 'boolean',
    },
+    'filename': {
+      alias: 'f',


"f" is already been used in the web-ext run command as a shortcut for "--firefox-binary", even if this is technically only available for the build command I would still prefer to possibly using a different single char for this option or alternatively we may also just don't specify a single char alias.

Also, I'm wondering if this is the reason for the failure triggered on the travis CI windows workers: https://travis-ci.org/mozilla/web-ext/jobs/604599391#L677
But it is surprising that it is only failing on windows and not on the travis CI linux workers as well.

That is an interesting error, yet I am not sure what causes it on Windows. It seems to me that leaving a single char alias empty would be a better choice in this case . I just can't think of any other intuitive alias for the filename option.

sure, omitting a single char alias for this option sounds good to me too.

Rob--W · 2019-10-30T13:43:24Z

src/cmd/build.js

-      messageFile, manifestData,
-    });
+  let packageName;
+  if (filename) {


The proposed implementation does not support variables as requested in #1335.

Let's undo the current change to this file, and do something like this:

filename = filename.replace( ... , ... ); const packageName = safeFilename(filename);

If you don't know exactly how the replacement should be implemented, look at the getDefaultLocalizedName method for an example. To start simple, you could support the variable names "name" and "version".

rpl

@birtony Apologies for missing your last round of questions (I thought that this PR was just waiting fro the changes mentioned by me and Rob in the previous review round).

I've replied inline to the your questions, and also added some additional comments related to the "filename template interpolation" part (previously mentioned in Rob's comment).

Let me know if you have all the needed details or if you have any other doubts or questions.

rpl · 2019-12-09T13:58:58Z

src/cmd/build.js

-    });
+  let packageName;
+  if (filename) {
+    packageName = `${filename}.zip`;


As Rob pointed out in his comment, the original request in #1335 is to also allow the user to specify some manifest properties to be interpolated into the final filename, and so here we should be calling a newly defined helper method, e.g. one with the following type signature:

function getPackageNameFromTemplate( filenameTemplate: string, manifestData: ExtensionManifest ): string

As Rob mentioned, we could use an approach similar to the one used internally by getDefaultLocalizedName, which should look like:

filenameTemplate.replace(/<<REGEXP>>/g, (match, manifestProperty) => { ... // <- find and return manifestPropertyValue (or throw if the property can't be found // or isn't a string) });

As suggested by Kumar in the issue comments, we could aim for template strings that look like: '{applications.gecko.id}-{version}.xpi'
And so the regexp should match on strings enclosed between the { and } characters and the enclosed string should only allow alphanumeric characters and the . as the property separator character.

rpl · 2019-12-09T14:01:02Z

tests/unit/test-cmd/test.build.js

@@ -72,6 +72,21 @@ describe('build', () => {
    });
  });

+  it('gives the correct custom name to an extension', () => {


Let's also unit test the new helper method that does just interpolate the template string given the manifest data object (so that we can easily cover more of the other scenarios, like the error scenarios where the helper method is meant to throw).

rpl · 2020-01-22T16:41:36Z

I'm closing this PR because it is not yet complete and in the meantime there are some conflicts (with changes landed in master in the meantime) that have to be solved.

If you still want to work on this and the issue has not been fixed yet, feel free to update this PR and mention me in a comment and I'll be happy to re-open this PR.

escapewindow · 2020-05-04T20:41:02Z

@birtony are you planning on picking this back up? If not, do you mind if I try addressing these review comments?

birtony · 2020-05-04T20:43:51Z

@escapewindow things got busy for me, so go ahead! Let me know if I could be of any help

rpl requested changes Oct 28, 2019

View reviewed changes

src/cmd/build.js Outdated Show resolved Hide resolved

feat: Added custom filename parameter to build cmd (#1335)

6fada80

birtony requested a review from rpl October 29, 2019 15:42

feat: Added unit test for filename param (#1335)

c40cc67

rpl requested changes Oct 30, 2019

View reviewed changes

Rob--W requested changes Oct 30, 2019

View reviewed changes

rpl requested changes Dec 9, 2019

View reviewed changes

rpl closed this Jan 22, 2020

escapewindow mentioned this pull request May 5, 2020

add --filename option to specify a filename template #1900

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #1335 Added parameter to build command to set custom filename #1741

Fixes #1335 Added parameter to build command to set custom filename #1741

birtony commented Oct 27, 2019 •

edited

coveralls commented Oct 28, 2019 •

edited

rpl left a comment

rpl Oct 30, 2019

birtony Oct 30, 2019 •

edited

rpl Oct 30, 2019

rpl Oct 30, 2019

birtony Oct 30, 2019

birtony Nov 19, 2019 •

edited

birtony Nov 19, 2019

rpl Dec 9, 2019

rpl Oct 30, 2019

birtony Nov 19, 2019

rpl Dec 9, 2019

Rob--W Oct 30, 2019

rpl left a comment

rpl Dec 9, 2019

rpl Dec 9, 2019

rpl commented Jan 22, 2020

escapewindow commented May 4, 2020

birtony commented May 4, 2020

Fixes #1335 Added parameter to build command to set custom filename #1741

Fixes #1335 Added parameter to build command to set custom filename #1741

Conversation

birtony commented Oct 27, 2019 • edited

coveralls commented Oct 28, 2019 • edited

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

birtony Oct 30, 2019 • edited

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

birtony Nov 19, 2019 • edited

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

rpl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpl commented Jan 22, 2020

escapewindow commented May 4, 2020

birtony commented May 4, 2020

birtony commented Oct 27, 2019 •

edited

coveralls commented Oct 28, 2019 •

edited

birtony Oct 30, 2019 •

edited

birtony Nov 19, 2019 •

edited