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

WebExtension migration #4864

Merged
merged 1 commit into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@6a68
Member

6a68 commented Sep 11, 2018

Having some trouble getting addons-linter to install, and had to manually delete and re-add package-lock.json (am I doing this wrong?), but this otherwise seems to work for me locally.

Things to note, from the actual commit:


Migrate Screenshots to pure WebExtension

  • The addons system, not the Screenshots addon, will be responsible for
    detecting changes to the 'extensions.screenshots.disabled' pref, and
    starting up or shutting down the Screenshots webextension when changes
    occur (bug 1488971).

  • The LibraryButton code has been moved from bootstrap.js into the
    embedded API. We'll be able to remove this code as soon as Library
    menu items can be declared from the WebExtension manifest (bug 1366389).

  • Move addon/webextension to webextension, update all the refs.

  • Update makefile and build tools.

  • Remove the script that deletes duplicate locales (en_CA) at build
    time, because this breaks running with web-ext, and there is code to
    handle duplicate locales inside export_mc.py already.

  • Still TODO: README files and docs directory not yet updated

@6a68

This comment has been minimized.

Show comment
Hide comment
@6a68

6a68 Sep 11, 2018

Member

Looks like Circle-CI had problems building addons-linter, and I had problems with that, too. I'm not sure if I broke something with the build tooling changes.

Member

6a68 commented Sep 11, 2018

Looks like Circle-CI had problems building addons-linter, and I had problems with that, too. I'm not sure if I broke something with the build tooling changes.

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Sep 11, 2018

Contributor

The addons-linter issue is caused by some kind of (inter)dependency problem. But mozilla/addons-linter#1617 is not helping either.

If we reinstall jpm for this branch, then addons-linter would start working again. With some digging, it seems like it's sign-addon from jpm that's needed. But since addons-linter doesn't appear to use sign-addon directly, maybe it's one of sign-addon's dependencies? Madness.

Contributor

chenba commented Sep 11, 2018

The addons-linter issue is caused by some kind of (inter)dependency problem. But mozilla/addons-linter#1617 is not helping either.

If we reinstall jpm for this branch, then addons-linter would start working again. With some digging, it seems like it's sign-addon from jpm that's needed. But since addons-linter doesn't appear to use sign-addon directly, maybe it's one of sign-addon's dependencies? Madness.

@chenba

chenba approved these changes Sep 11, 2018

LGTM. Just gotta figure out what to do about the addons-linter issue.

@6a68

This comment has been minimized.

Show comment
Hide comment
@6a68

6a68 Sep 11, 2018

Member

Nice sleuthing! I'm fine to re-add jpm as a dev dependency. Might even be able to get rid of those awful package-lock.json commits

Member

6a68 commented Sep 11, 2018

Nice sleuthing! I'm fine to re-add jpm as a dev dependency. Might even be able to get rid of those awful package-lock.json commits

@chenba

This comment has been minimized.

Show comment
Hide comment
@chenba

chenba Sep 11, 2018

Contributor

Cannot find module 'regenerator-runtime' is the actual error. And it's a webpack dependency.

Contributor

chenba commented Sep 11, 2018

Cannot find module 'regenerator-runtime' is the actual error. And it's a webpack dependency.

@6a68

This comment has been minimized.

Show comment
Hide comment
@6a68

6a68 Sep 11, 2018

Member

@aswan Hey, would you mind taking a look at this PR? Particularly looking for feedback on the API implementation: https://github.com/mozilla-services/screenshots/pull/4864/files#diff-8e1f4fd10b786635bc10f97bcbec60e3

Member

6a68 commented Sep 11, 2018

@aswan Hey, would you mind taking a look at this PR? Particularly looking for feedback on the API implementation: https://github.com/mozilla-services/screenshots/pull/4864/files#diff-8e1f4fd10b786635bc10f97bcbec60e3

@6a68

This comment has been minimized.

Show comment
Hide comment
@6a68

6a68 Sep 11, 2018

Member

Looks like I've got a rebase to do before this lands. @ianb do you have any comments on this PR?

Member

6a68 commented Sep 11, 2018

Looks like I've got a rebase to do before this lands. @ianb do you have any comments on this PR?

Migrate Screenshots to pure WebExtension
* The addons system, not the Screenshots addon, will be responsible for
detecting changes to the 'extensions.screenshots.disabled' pref, and
starting up or shutting down the Screenshots webextension when changes
occur (bug 1488971).

* The LibraryButton code has been moved from bootstrap.js into the
embedded API. We'll be able to remove this code as soon as Library
menu items can be declared from the WebExtension manifest (bug 1366389).

* Move addon/webextension to webextension, update all the refs.

* Update makefile and build tools.

* Remove the script that deletes duplicate locales (en_CA) at build
time, because this breaks running with web-ext, and there is code to
handle duplicate locales inside export_mc.py already.

* Keep `jpm` because `addons-linter` seems to depend on it

* Still TODO: README files and docs directory not yet updated

@6a68 6a68 referenced this pull request Sep 12, 2018

Open

Remove jpm #4873

@6a68 6a68 merged commit 010867e into mozilla-services:master Sep 12, 2018

2 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@6a68 6a68 deleted the 6a68:webextension-migration branch Sep 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment