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: run command handles bootstap addons #1019

Closed
wants to merge 1 commit into from

Conversation

chrmod
Copy link
Contributor

@chrmod chrmod commented Aug 1, 2017

on top of: #1018
and this PR, web-ext can be use to run boostrapped/system addons.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 99.887% when pulling 910baff on chrmod:bootstap-support into 65d9459 on mozilla:master.

@kumar303
Copy link
Contributor

kumar303 commented Aug 1, 2017

Hi. Thanks for taking the time to submit a patch. We do not intend to support bootstrap add-ons with web-ext because there are too many special cases that we'd need to accommodate.

I'm going to close this issue but you can still work with us to help you solve the issue at hand. I suggest trying some experimental patches (like this one) in your local build of web-ext and see how far you get. Are you able to run the extension? What needed to change? Perhaps this can guide us in extracting some useful parts of web-ext into a new bootstrap extension runner or exposing some new things programmatically so you can write a script.

Also, you may wish to try out jpm run which I believe supports bootstrap extensions.

@kumar303 kumar303 closed this Aug 1, 2017
@chrmod
Copy link
Contributor Author

chrmod commented Aug 2, 2017

got the web-ext run command working fine for bootstrap addon with those 3 commits:

First two you know already, the last one makes it possible to reload the extension on code changes in case web-ext gets integrated with other build tool.

My team should start to work with it by Tomorrow, so we will get some feedback.

As changes are so minimal I would rather prefer to keep using web-ext, but if you really don't want to support bootstrap/system addons, that we can discuss, which part of code we can extract to separate package.

@kumar303
Copy link
Contributor

kumar303 commented Aug 2, 2017

How many tests are failing with those patches? I'd have to look closer at the implications of changing this return value.

As for allowing invalid manifests, this is just going to cause pain and sorrow for people who have a syntax error in their manifest. Supporting invalid WebExtension manifests (i.e. bootstrap add-ons) is going to be tricky and it will be hard to keep up support for that as the app changes. I would possibly accept a patch that allows you to use dependency injection like this:

// Set up a stub in your script:
const getFakeManifest: () => ({
  name: '',
  version: '',
});
// Patch `run()` to accept the stub function like this:
run(params, {getValidatedManifest: getFakeManifest})
  .then(...);

@chrmod
Copy link
Contributor Author

chrmod commented Aug 3, 2017

there was no failing test after changing run return type. This PR makes the change and adds some tests #1027

Can we also have this one merged #1018 ?

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

3 participants