Skip to content

Conversation

tofumatt
Copy link
Contributor

@tofumatt tofumatt commented Jun 29, 2016

Fixes #325

r?

I tested signing and it still a. works with the zip (as expected) and b. sends down an XPI, which is expected behaviour.

Tested web-ext run and everything still worked as normal too.

@tofumatt
Copy link
Contributor Author

I think this should also close #119, as it will package a ZIP file Firefox will load and (presumably) Chrome will load/accept.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 424998c on export-zip-files-instead-of-xpi-325 into 830065b on master.

@tofumatt
Copy link
Contributor Author

Just uploaded the ZIP file web-ext build output and the Chrome Store accepted the upload and it worked, so I'd say #119 is closed by this, yup.

screenshot 2016-06-29 13 29 38

@kumar303
Copy link
Contributor

Cool! I'm taking a look now.

One thing we've been doing is making each commit message auto-changelog-ready so could you rename the message to "feat: Output ZIP files instead of XPI files" ?

Also, the commit message itself should not mention the github issue (only the pull request should) otherwise every time you rebase and push, it will spam the issue thread (which gets messy).

@tofumatt
Copy link
Contributor Author

tofumatt commented Jun 29, 2016 via email

@tofumatt tofumatt force-pushed the export-zip-files-instead-of-xpi-325 branch from 424998c to 07ff822 Compare June 29, 2016 16:06
@tofumatt
Copy link
Contributor Author

Fixed both things!

let extension = 'zip';
let packageName = safeFileName(
`${manifestData.name}-${manifestData.version}.xpi`);
`${manifestData.name}-${manifestData.version}.${extension}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

might as well just make this ${manifestData.name}-${manifestData.version}.zip -- no need for a variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 07ff822 on export-zip-files-instead-of-xpi-325 into 830065b on master.

assert.equal(filter.wantFile('manifest.json'), false);
});

Copy link
Contributor

Choose a reason for hiding this comment

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

heh. I fixed the eslint rule for this in another PR that hasn't landed yet. I was like wut, how did that even get committed? 🎯

Copy link
Contributor Author

@tofumatt tofumatt Jun 29, 2016

Choose a reason for hiding this comment

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

And my editor just deletes trailing whitespace so I didn't even notice I committed it 😄

Should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, your change is correct. Thanks.

@kumar303
Copy link
Contributor

the Chrome Store accepted the upload

highfive

@kumar303
Copy link
Contributor

At first I was wondering if we should make an option to output to .xpi if desired but I can't really think of any reason to :)

Looks good!

@tofumatt tofumatt merged commit b41c338 into master Jun 29, 2016
@tofumatt tofumatt deleted the export-zip-files-instead-of-xpi-325 branch June 29, 2016 17:03
@kumar303
Copy link
Contributor

oh, heh, I was waiting for fixing up the extraneous variable. All good though, no need for an additional patch. Thanks for getting this in.

@kumar303 kumar303 mentioned this pull request Jun 29, 2016
@tofumatt
Copy link
Contributor Author

Shoot! I made the commit and didn't push it. I am dumb.

I'll make another patch, I hate silly extra lines.

@kumar303
Copy link
Contributor

ok, if you do, prefix the commit message with chore: so that it gets excluded from the auto-changelog. It's not exactly a chore but hey.

@tofumatt
Copy link
Contributor Author

tofumatt commented Jun 29, 2016 via email

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.

3 participants