feat: web-ext create simple version #771

saintsebastian · 2017-01-23T19:37:09Z

fixes #540

I came up with a couple of question about this issue:

behaviour of web-ext new, at the moment name of the current directory is used to generate the manifest as the name of the addon, maybe user would rather expect web-ext new name which would create a name folder and manifest file inside of it
the current structure and content of default manifest.json may not be perfect, I'd really like to know your opinion on what elements are necessary

coveralls · 2017-01-23T19:43:01Z

Coverage decreased (-0.7%) to 99.27% when pulling 37d4ed3 on saintsebastian:feat-540 into 7ad5c2a on mozilla:master.

coveralls · 2017-01-23T19:52:21Z

Coverage decreased (-0.7%) to 99.27% when pulling 68e6143 on saintsebastian:feat-540 into 7ad5c2a on mozilla:master.

coveralls · 2017-01-24T13:17:16Z

Coverage remained the same at 100.0% when pulling 5229109 on saintsebastian:feat-540 into 7ad5c2a on mozilla:master.

coveralls · 2017-01-24T13:27:09Z

Coverage remained the same at 100.0% when pulling 5229109 on saintsebastian:feat-540 into 7ad5c2a on mozilla:master.

rpl

Hi @saintsebastian
Thanks for getting this started!

We definitely need to make this command less destructive (e.g. when the target directory already exists and it contains the files that are going to be overwritten), we should also retrieve the target dir from the parameters instead of assuming it to be the current work dir by default (which is a pretty dangerous default for a command that is going to potentially overwrite existent files).

It is also a bit unfortunate that we cannot name the property exported by src/cmd/index.js as the command name (which is currently new), I'm wondering if we should opt to rename the command (e.g. into web-ext init), let see what @kumar303 thinks about it.

rpl · 2017-02-05T16:19:04Z

src/cmd/index.js

+import newCommand from './new';

-export default {build, lint, run, sign};
+export default {build, lint, run, sign, newCommand};


ouch, this is so unfortunate :-(
clearly and for obvious reason we cannot (and we shouldn't) use new as a property name here (we can force if to be valid if we quote the key, e.g. {...,"new": newCommand} but I'm not sure that it would make it nicer or less confusing)

@kumar303 how do we prefer to make this nicer? I currently see two options:

we can choose a different name for the command (e.g. web-ext init?)

we can choose to change the convention used to name the properties here for all the commands (e.g. {cmdBuild, cmdLint, ..., cmdNew})

we could also quote all the properties of this export (e.g. {"build": ..., "new": ...}) and always referring to this properties using the string key, but I'm not sure that I like it (I would probably vote for renaming this command to init).

I agree web-ext init is a better option, I'll proceed with this changes. Unless @kumar303 disagrees.

I think renaming the command is the best solution since it will allow us to match the name of the function to the command exactly.

Here are some ideas along with how they might be mnemonically associated in the developer's mind:

web-ext create

"Create a new web extension"

web-ext start

"Start developing a new web extension"

One downside is that npm start is a convention which usually applies more to starting a server or a test suite, depending on the project.

web-ext init

"Initialize a new web extension"

I'm not totally opposed to this, it just seems like an unnecessary abbreviation to me when we have options for shorter words.

Any of these are fine by me so pick your favorite :) I would probably choose create myself.

rpl · 2017-02-05T16:28:50Z

src/cmd/new.js

+
+const manifest = {
+  background: {
+    scripts: [],


we definitely need to discuss a bit about what it makes sense to include in the minimal generated webextension,
and I think that starting to review this manifest skeleton is definitely a good starting point to make this discussion to happen, e.g.

we should not generate a manifest with both the "background.scripts" and "background.page" properties, I think that including "background.script" with just 1 script named "background.js" in the scripts array is enough.

in my opinion, while it could make sense to include a browser_action, just as a common UI element included in an addon, I'd remove the page_action section, its syntax is pretty similar to the browser_action property and I think that it is not very common to use both in a single addon.

in my opinion a content script should be added to the skeleton, especially because I think that it is pretty common for a WebExtension to include one to manipulate the content.

I also think that we should probably generate the background and content script javascript files (even just two files with a single console.log call, e.g. console.log("ADDON NAME background page loaded") or something similar).

@kumar303 what are your opinion about what we should include in this minimal generated webextension?

rpl · 2017-02-05T16:37:30Z

src/cmd/new.js

+};
+
+export default async function newCommand(): Promise<void> {
+  const path = process.cwd();


I'm not sure if we should default it to process.cwd(), but I'm pretty sure that we need to make this command less dangerous (e.g. we should check that we are not going to overwrite existent files in the target dir by mistake)

I would assume that the path is the yargs "rest", the one that are in the argv._ property after yargs has completed the command and options parsing.

Retrieving the path from argv._ is probably something that should be done in src/program.js, and then we can pass it to the this command in its parameters.
(and we should also be sure that we raise and log an error if there are more that one element in the argv._ property, or if there is no selected target path at all)

rpl · 2017-02-05T16:43:05Z

src/cmd/new.js

+  const path = process.cwd();
+  const title = path.match(/[A-z0-9_-]+$/);
+  if (Array.isArray(title)) {
+    manifest.name = title[0];


we can probably make this part (fill the customized properties of the manifest skeleton) nicer if we wrap the manifest skeleton into a utility method, e.g.:

function generateManifest({title}) { return { manifest_version: 2, name: `${title} (name)`, description: `${title} (description)`, ... browser_action: { default_title: `${title} (browserAction)`, ... }, ... };

and then we can just use it like:

const generatedManifest = createManifest({title: initTargetPath}); ... await fs.writeFile('manifest.json', generateManifest, 'utf8');

rpl · 2017-02-05T16:45:56Z

src/cmd/new.js

+    manifest.name = title[0];
+    manifest.page_action.default_title = title[0];
+    manifest.browser_action.default_title = title[0];
+    var json = JSON.stringify(manifest, null, 2);


no var, we should use only const and let.

rpl · 2017-02-05T16:47:58Z

src/cmd/new.js

+    manifest.browser_action.default_title = title[0];
+    var json = JSON.stringify(manifest, null, 2);
+    try {
+      await fs.writeFile('manifest.json', json, 'utf8');


Like I wrote above, we should not overwrite any existent file in the target directory (at least not without an additional user confirmation, e.g. an additional cli option like --allow-overwrite)

rpl · 2017-02-05T16:51:23Z

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

+describe('new', () => {
+
+  after(() => {
+    process.chdir(homeDir);


Like I wrote above, we should pass the target path explicitly to this command (even if the default value is the current working dir), once we change the command to get the target path from its parameters, we can remove any process.chdir magic from this unit test (I really prefer that the unit tests, as the code in the web-ext sources, never change the current working dir... very bad things happens if you lose track of which one is the current working dir and what has changed it :-)).

saintsebastian · 2017-02-14T22:02:29Z

@rpl To avoid creating a file in working directory I wrote test to chdir into temporary folder before and after test, which may not be the best solution. Also I am still figuring out how to test usage error because flow really doesn't like process.stdin.isTTY method.

coveralls · 2017-03-10T15:33:11Z

Coverage remained the same at 100.0% when pulling 4349d8e on saintsebastian:feat-540 into 8c53c29 on mozilla:master.

coveralls · 2017-03-10T15:39:47Z

Coverage remained the same at 100.0% when pulling 4349d8e on saintsebastian:feat-540 into 8c53c29 on mozilla:master.

rpl · 2017-03-24T18:01:56Z

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

+
+  it('creates files including manifest with correct name', () => withTempDir(
+    (tmpDir) => {
+      process.chdir(tmpDir.path());


Like discussed, we should change the current work dir during the tests (and it should fix the failures of these tests on windows) here, in the afterEach and elsewhere in this test file.

rpl · 2017-03-24T18:03:18Z

package.json

    "load-grunt-tasks": "3.5.2",
    "mocha": "3.2.0",
    "mocha-multi": "0.10.0",
+    "mock-stdin": "0.3.1",


We can probably reuse the mock that we created for #746 and do not include this as a devDependencies.

rpl · 2017-03-24T18:09:02Z

src/cmd/create.js

+const log = createLogger(__filename);
+const defaultAsyncMkdirp = promisify(mkdirp);
+
+export type CreateParams = {|


we should split these properties into two parameters and flow types like we do for the other commands:

the first one should be named SomethingParams (e.g. CreateCmdParams) and it should only contains what is supposed to be in the regular parameters of the command (e.g. dirPath in this case)

the second one should be named SomethingOptions (e.g. CreateCmdOptions) and it should only contains the optional injected dependencies (stdin? in this case), which are used mostly to override the dependencies in the unit tests

rpl · 2017-03-24T18:15:53Z

src/cmd/create.js

+    stdin = process.stdin,
+  }: CreateParams
+): Promise<void> {
+  const targetPath = path.join(process.cwd(), dirPath);


I'm thinking that instead of using process.cwd() directly here, it would be better to turn it into a parameter (which defaults to process.cwd() if missing) and/or into an injected dependency (e.g. something like function getBaseDir(baseDir) { return baseDir || process.cwd(); }).

This way, the command can provide much more control to the caller if needed, which should be helpful both in the unit tests and while using web-ext as a library.

coveralls · 2017-06-06T11:22:40Z

Coverage remained the same at 100.0% when pulling 37e83c7 on saintsebastian:feat-540 into 50528bf on mozilla:master.

coveralls · 2017-06-06T11:28:23Z

Coverage remained the same at 100.0% when pulling da84e7b on saintsebastian:feat-540 into 50528bf on mozilla:master.

rpl · 2017-07-01T00:02:17Z

Closing this PR, we have moved this feature into its own create-webextension npm package and github repo for now.

saintsebastian requested a review from jaredhirsch January 23, 2017 19:37

muffinresearch added the state: pull request ready label Jan 23, 2017

saintsebastian requested review from rpl and removed request for jaredhirsch January 23, 2017 19:37

saintsebastian assigned saintsebastian and rpl Jan 23, 2017

feat: web-ext new automatic manifest

68e6143

saintsebastian force-pushed the feat-540 branch from 37d4ed3 to 68e6143 Compare January 23, 2017 19:46

test: web-ext new basic test

5229109

saintsebastian changed the title ~~Feat: web-ext new simple version~~ feat: web-ext new simple version Jan 24, 2017

saintsebastian closed this Jan 24, 2017

saintsebastian reopened this Jan 24, 2017

muffinresearch added state: pull request ready and removed state: pull request ready labels Jan 24, 2017

rpl requested changes Feb 9, 2017

View reviewed changes

saintsebastian force-pushed the feat-540 branch 2 times, most recently from bd8ca10 to 91c6a73 Compare February 14, 2017 21:09

feat: cli interface and correct exit

647c0a1

saintsebastian force-pushed the feat-540 branch from 8123fbb to 647c0a1 Compare February 14, 2017 21:45

saintsebastian added 2 commits March 10, 2017 16:25

fix: better while loop and testing

4cdc8d7

Merge branch 'master' into feat-540

4349d8e

saintsebastian closed this Mar 10, 2017

saintsebastian reopened this Mar 10, 2017

muffinresearch removed the state: pull request ready label Mar 10, 2017

muffinresearch added the state: pull request ready label Mar 10, 2017

saintsebastian changed the title ~~feat: web-ext new simple version~~ feat: web-ext create simple version Mar 24, 2017

rpl requested changes Mar 24, 2017

View reviewed changes

Merge branch 'master' into feat-540

37e83c7

saintsebastian force-pushed the feat-540 branch 4 times, most recently from 950d5dc to 37e83c7 Compare June 6, 2017 12:49

test: adapted for windows fs

4e8e438

rpl closed this Jul 1, 2017

muffinresearch removed the state: pull request ready label Jul 1, 2017

saintsebastian mentioned this pull request Jul 10, 2017

web-ext new: create the directory layout for an extension #540

Open

snyk-bot mentioned this pull request Jul 6, 2021

[Snyk] Upgrade sign-addon from 3.3.0 to 3.5.0 terrorizer1980/web-ext#3

Open

feat: web-ext create simple version #771

feat: web-ext create simple version #771

Uh oh!

Conversation

saintsebastian commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rpl left a comment

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!

saintsebastian commented Feb 14, 2017

Uh oh!

coveralls commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Mar 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

coveralls commented Jun 6, 2017

Uh oh!

coveralls commented Jun 6, 2017

Uh oh!

rpl commented Jul 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saintsebastian commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 23, 2017 •

edited

Loading

coveralls commented Jan 24, 2017 •

edited

Loading

coveralls commented Jan 24, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading

coveralls commented Mar 10, 2017 •

edited

Loading