Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6fada80
feat: Added custom filename parameter to build cmd (#1335)
birtony Oct 27, 2019
c40cc67
feat: Added unit test for filename param (#1335)
birtony Oct 28, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 53 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 26 additions & 16 deletions src/cmd/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ export type PackageCreatorParams = {|
fileFilter: FileFilter,
artifactsDir: string,
overwriteDest: boolean,
showReadyMessage: boolean
showReadyMessage: boolean,
filename?: string,
|};

export type LocalizedNameParams = {|
Expand Down Expand Up @@ -115,6 +116,7 @@ export async function defaultPackageCreator(
artifactsDir,
overwriteDest,
showReadyMessage,
filename,
}: PackageCreatorParams,
{
eventToPromise = defaultEventToPromise,
Expand All @@ -132,22 +134,27 @@ export async function defaultPackageCreator(
filter: (...args) => fileFilter.wantFile(...args),
});

let extensionName: string = manifestData.name;

let {default_locale} = manifestData;
if (default_locale) {
default_locale = default_locale.replace(/-/g, '_');
const messageFile = path.join(
sourceDir, '_locales',
default_locale, 'messages.json'
);
log.debug('Manifest declared default_locale, localizing extension name');
extensionName = await getDefaultLocalizedName({
messageFile, manifestData,
});
let packageName;
if (filename) {
Copy link
Member

@Rob--W Rob--W Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

packageName = `${filename}.zip`;
Copy link
Member

@rpl rpl Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

@rpl rpl Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@birtony birtony Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@birtony birtony Nov 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
    // ...
}

Copy link
Author

@birtony birtony Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@rpl rpl Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Member

@rpl rpl Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
let extensionName: string = manifestData.name;

let {default_locale} = manifestData;
if (default_locale) {
default_locale = default_locale.replace(/-/g, '_');
const messageFile = path.join(
sourceDir, '_locales',
default_locale, 'messages.json'
);
log.debug('Manifest declared default_locale, localizing extension name');
extensionName = await getDefaultLocalizedName({
messageFile, manifestData,
});
}
packageName = safeFileName(
`${extensionName}-${manifestData.version}.zip`);
}
const packageName = safeFileName(
`${extensionName}-${manifestData.version}.zip`);
const extensionPath = path.join(artifactsDir, packageName);

// Added 'wx' flags to avoid overwriting of existing package.
Expand Down Expand Up @@ -187,6 +194,7 @@ export type BuildCmdParams = {|
asNeeded?: boolean,
overwriteDest?: boolean,
ignoreFiles?: Array<string>,
filename?: string,
|};

export type BuildCmdOptions = {|
Expand All @@ -206,6 +214,7 @@ export default async function build(
asNeeded = false,
overwriteDest = false,
ignoreFiles = [],
filename,
}: BuildCmdParams,
{
manifestData,
Expand All @@ -231,6 +240,7 @@ export default async function build(
artifactsDir,
overwriteDest,
showReadyMessage,
filename,
});

await prepareArtifactsDir(artifactsDir);
Expand Down
9 changes: 9 additions & 0 deletions src/program.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,15 @@ Example: $0 --help run.
default: true,
type: 'boolean',
},
'filename': {
alias: 'f',
Copy link
Member

@rpl rpl Oct 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Author

@birtony birtony Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@rpl rpl Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

describe: 'Name of the created extension package.',
default: undefined,
normalize: true,
demandOption: false,
requiresArg: false,
type: 'string',
},
});

program
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/test-cmd/test.build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,21 @@ describe('build', () => {
});
});

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

@rpl rpl Dec 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return withTempDir(
(tmpDir) =>
build({
sourceDir: fixturePath('minimal-web-ext'),
artifactsDir: tmpDir.path(),
filename: 'test',
})
.then((buildResult) => {
assert.match(buildResult.extensionPath,
/test\.zip$/);
})
);
});

it('gives the correct name to a localized extension', () => {
return withTempDir(
(tmpDir) =>
Expand Down