Skip to content
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.

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

Convert bin/xpi.sh into cross-env Node type script? #60

Closed
pdehaan opened this issue Mar 20, 2018 · 4 comments · Fixed by #73
Closed

Convert bin/xpi.sh into cross-env Node type script? #60

pdehaan opened this issue Mar 20, 2018 · 4 comments · Fixed by #73

Comments

@pdehaan
Copy link
Contributor

pdehaan commented Mar 20, 2018

Currently /bin/xpi.sh is some pretty hardcore Bash-ism. Not sure if it'd be easier to rewrite as Node, or if that'd just be a lot more verbose for not a lot of gain. Not sure how many shield study authors are on a Windows environment, or how much customizing people are doing to the xpi.sh file to tweak for specific projects.

I took a quick stab at the conversion, which is a bit more verbose than the Bash version, but maybe is something we could consider publishing to npm if we can agree on the correct dials and knobs.

For example, this will create a /dist/addon-2.0.0.xpi file (but is easy enough to remove the version number from the filename and always write as dist/addon.xpi):

const {readFileSync, writeFileSync, mkdtempSync} = require("fs");
const path = require("path");
const {promisify} = require("util");

const glob = require("glob").sync;
const Mustache = require("mustache");
const rimraf = require("rimraf").sync;
const cpr = promisify(require("cpr"));
const zipdir = promisify(require("zip-dir"));

const pkg = require("../addon.json");

async function makeXpi(options) {
  const BASE_DIR = path.join(__dirname, "..");

  options = Object.assign({}, {
    templateData: "package.json",
    extensionDir: "extension",
    templateDir: "templates",
    xpiName: process.env.XPI || "addon.xpi",
    verbose: process.env.VERBOSE === "true"
  }, options);
  // If the template data option looks like a string, assume it's a file path.
  if (typeof options.templateData === "string") {
    options.templateData = require(path.join(BASE_DIR, options.templateData));
  }
  options.extensionDir = path.join(BASE_DIR, options.extensionDir);
  options.templateDir = path.join(BASE_DIR, options.templateDir);
  options.xpiPath = path.join(BASE_DIR, options.xpiDir, options.xpiName);

  const logFile = (file) => options.verbose ? console.log(`Added ${file}`) : null;
  const copyTemplate = (tmpl, data, dest) => {
    const tmplFile = readFileSync(tmpl);
    // Remove trailing ".mustache" extension.
    const outFile = path.basename(tmpl, path.extname(tmpl));
    const output = Mustache.render(tmplFile.toString(), data);
    return writeFileSync(path.join(dest, outFile), output);
  };

  try {
    options.tmpDir = mkdtempSync(path.join(__dirname, options.templateData.addon.id));
    glob(path.join(options.templateDir, "*.mustache"))
      .forEach(file => copyTemplate(file, options.templateData, options.tmpDir));
    await cpr(options.extensionDir, options.tmpDir);
    await zipdir(options.tmpDir, {saveTo: options.xpiPath, each: logFile});
  } catch (err) {
    console.error(err);
  } finally {
    rimraf(options.tmpDir);
  }
}

makeXpi({
  extensionDir: "extension",
  templateData: pkg, // or "addon.json",
  templateDir: "templates",
  xpiDir: "dist",
  xpiName: `addon-${pkg.addon.version}.xpi`, // or "addon.xpi" if we don't want version numbers in the file name.
});

I think the one thing it doesn't currently account for is copying arbitrary files before zipping, a la:

cp node_modules/pioneer-utils/dist/PioneerUtils.jsm "${DEST}"

But we can do something like require.resolve() to get paths to file names we want to copy from ./node_modules/:

$ node -e "console.log(require.resolve('pioneer-utils/dist/PioneerUtils.jsm'))"
# /Users/pdehaan/tmp/node_modules/pioneer-utils/dist/PioneerUtils.jsm

We could probably just add some extraFiles[] array to the options Object which lists arbitrary files to copy over from ./node_modules/ or wherever.

Oh, and there isn't an easy way currently to list files you DON'T want in the generated XPI/ZIP file. I think cpr (after cp -r, presumably) supports a filter function/regex for preventing certain files from being copied over. I was hoping we could use web-ext to create the XPI, but not sure if that is fully supported.

Another option is looking to see if we can replace a few of these dependencies with shelljs. It MAY be able to replace at least one of these dependencies:

const glob = require("glob").sync; // `shelljs.ls()`?
const rimraf = require("rimraf").sync; // `shelljs.rm()`?
const cpr = promisify(require("cpr")); // `shelljs.cp()`?

I'm probably overlooking something obvious and there is a much better solution (like Webpack or something) for copying/moving/templating files. I'm a bit reluctant to add Grunt or Gulp just for builds, as I don't know how simple it'd be to publish some "make-xpi" module that can be shared between projects.

@gregglind
Copy link
Contributor

Thanks for looking into this.

I LIVE HARDCORE BASH.

But... legacy addons are dead, and I have a replacement script!

web-ext build

Under WebExtensionsExperiments all this build garbage just goes away.

@gregglind
Copy link
Contributor

(In the past, we have also MADE DECISIONS about a few of these things.

  1. We de-grunted once already :)
  2. We decided that if the files are there, they are going into the xpi, rather than explicit list.

Most of what you suggest sound like really good things to check that web-ext build knows about / can do. Will check there.

@pdehaan
Copy link
Contributor Author

pdehaan commented Mar 20, 2018

Not all the build stuff, i reckon... We'd still need the mustache transforms, and copying stray files like PioneerUtils.jsm from node_modules into somewhere.
Although... maybe some of those could be cured by clever .gitignore config and a prebuild npm hook.

Awesome, I'll wait until we shift to WEE and I can tinker w/ the new and improved build scripts.

@motin
Copy link
Contributor

motin commented Jan 30, 2019

Closing. This was fixed in #73

@motin motin closed this as completed Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants