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

feat: Add file extension option to web-ext build #392

Closed
wants to merge 1 commit into from

Conversation

rhelmer
Copy link

@rhelmer rhelmer commented Jul 26, 2016

Fixes #393

@coveralls
Copy link

coveralls commented Jul 26, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 10fb598 on rhelmer:add-file-ext-option-to-build into 3d7445e on mozilla:master.

@@ -182,6 +182,10 @@ Example: $0 --help run.
describe: 'Watch for file changes and re-build as needed',
type: 'boolean',
},
'extension': {
describe: 'Change the file extension from .zip to something else',
Copy link
Contributor

Choose a reason for hiding this comment

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

To make it clear that the value should not include a dot, maybe add another sentence: "Example: --extension=xpi"

@kumar303
Copy link
Contributor

Thanks! I figured someone would ask for this eventually :) Comments added.

@kumar303
Copy link
Contributor

kumar303 commented Aug 4, 2016

@rhelmer I guess you found a workaround but did you still want to get this in?

@rhelmer
Copy link
Author

rhelmer commented Aug 4, 2016

Yep, sorry for the delay.

@rhelmer rhelmer force-pushed the add-file-ext-option-to-build branch from 10fb598 to 3556526 Compare August 5, 2016 17:56
@coveralls
Copy link

coveralls commented Aug 5, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 3556526 on rhelmer:add-file-ext-option-to-build into 40d7abc on mozilla:master.


it('throws an error when invalid extension is provided', () => {
let failed = false;
try {
Copy link
Contributor

Choose a reason for hiding this comment

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

all callers will expect to treat build()'s return value as a promise so I think it's time to wrap the first part of that function in a promise:

return new Promise(
  (resolve) => {
    if (!allowedExtension.test(fileExtension)) {
      throw new Error('...');
    }
    resolve();
  })
  .then(() => prepareArtifactsDir(artifactsDir));

@rpl is embarking on an epic patch to switch us to async/await! #411

Copy link
Contributor

Choose a reason for hiding this comment

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

after that change, here's the pattern we use to check for promise rejections.

@kumar303
Copy link
Contributor

Now that we landed improved static typing, this patch will need a rebase and considerable cleanup. If you want to close it and just wait until someone needs the feature, that's fine. I am happy to review it again if you want to resurrect it though.

@rhelmer
Copy link
Author

rhelmer commented Aug 12, 2016

@kumar303 well I am using it, happy to rebase and cleanup :)

@kumar303
Copy link
Contributor

Hi @rhelmer . We landed a big change on master that switches to async/await instead of promises. This should make your patch easier though :) The tests still use promises (for now).

@kumar303
Copy link
Contributor

kumar303 commented Jan 3, 2017

We're getting a bit overwhelmed with pull requests so I'm trying to clear some mental space on the tracker. Feel free to re-open this if you feel like working on it!

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 this pull request may close these issues.

None yet

4 participants