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

Setup publishing to AMO #180

Merged
merged 10 commits into from Jun 16, 2019

Conversation

Projects
None yet
4 participants
@notlmn
Copy link
Collaborator

commented Jun 11, 2019

Setup Travis to publish extension to AMO. Follows the same publishing pattern as Refined GitHub except for the Git tags part.

The test extension resides at https://addons.mozilla.org/en-US/firefox/addon/nfg/.

Firefox support as of now is a bit finicky, like requesting permissions don't work yet, rest of the extension works fine though. Can be tested using the link above.

Fixes #137 (partially, for now), should make some Firefox users happy.

Things to do

  • Handle Firefox' weird callback handling behavior
  • Fix tests (possibly rewrite them)
  • Setup daily publishing like RGH, not publish on push (can use hotfix tags for emergency releases, similar to RGH)

Disclaimer

This is going to be a long PR, so Firefox users, hold your horses and if you can, provide feedback by testing the debug published to AMO.

notlmn added some commits Jun 11, 2019

@notlmn notlmn requested review from sindresorhus and YurySolovyov Jun 11, 2019

Show resolved Hide resolved source/background.js Outdated
Show resolved Hide resolved source/manifest.json Outdated
Show resolved Hide resolved source/manifest.json Outdated
Show resolved Hide resolved .babelrc Outdated
Show resolved Hide resolved .babelrc Outdated
@ravanscafi

This comment has been minimized.

Copy link

commented Jun 11, 2019

I was able to install using provided URL 🎉

However, I'm unable to mark the check "Show desktop notifications", even when opening the "new page".

image

(and also I think that the link to new page should have a about="blank")

I'm willing to help, if it's wanted :)

@sindresorhus

This comment has been minimized.

Copy link
Owner

commented Jun 11, 2019

I think we can actually just drop Babel. esm takes care of import/export.

@ravanscafi

This comment has been minimized.

Copy link

commented Jun 11, 2019

Clicking on the button on bar has no effect 😢
(But it's correctly showing notification count badge tho).

Also, CSS is not loaded on options:

Security Error: Content at moz-extension://b8c0ffea-a9f9-4f79-87b7-0e0bd1fcbb2d/options.html may not load or link to chrome://mozapps/skin/extensions/extensions.css.
Show resolved Hide resolved package.json Outdated
Show resolved Hide resolved source/background.js Outdated
@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

However, I'm unable to mark the check "Show desktop notifications", even when opening the "new page".

Clicking on the button on bar has no effect 😢
(But it's correctly showing notification count badge tho).

These are all the same issues, for Firefox asynchronous calls from an event listener callback are not part of the same call-stack as the callback itself. So browser.permissions.request() fails saying that it should only called as part of user input, but as it is inside another async function, this fails.

Will have to rewrite a part of that logic, or someone can suggest me a workaround for this behavior in Firefox.

@bfred-it
Copy link

left a comment

I think the solution is to not query the permissions. When you call request I think it will automatically pass, if we already have the permission, it won’t ask again. Untested but it wouldn’t make sense otherwise.

Show resolved Hide resolved source/lib/permissions-service.js
Show resolved Hide resolved .babelrc Outdated
@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

I think we can actually just drop Babel. esm takes care of import/export.

Correct me if I'm wrong but Babel is a compile time dependency, whereas esm is a runtime dependency. Better to have the previous one than the later one as we are already using webpack to bundle stuff and little to no transpiling happening anyway.

@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

I think the solution is to not query the permissions. When you call request I think it will automatically pass, if we already have the permission, it won’t ask again. Untested but it wouldn’t make sense otherwise.

The thing is that Firefox won't let you request permission in the first place, the function has to be called as part of a user interaction (which it already is), but according to Firefox, async function inside callback would not be considered as the same call-stack so Firefox thinks that the function is being called outside of user interaction. That is what I was explaining #180 (comment).

@bfred-it

This comment has been minimized.

Copy link

commented Jun 13, 2019

We target the latest browsers, we can use the latest features without having to compile them down to ES5

@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 13, 2019

Browserlist configuration in the .babelrc files makes sure that we are not compiling down to ES5. For example, the output file has async functions, they are not transpiled down to use generator functions.

I'll test dropping Babel altogether as we don't need transpiling anyway, and as we are bundling with webpack, I don't thing we need esm too.

@bfred-it

This comment has been minimized.

Copy link

commented Jun 13, 2019

as we don't need transpiling anyway,

That’s what I’m saying 😅
Babel was only there to add esm support but this PR introduced a whole preset-env

Show resolved Hide resolved source/lib/permissions-service.js
Show resolved Hide resolved source/lib/tabs-service.js Outdated

notlmn added some commits Jun 14, 2019

@bfred-it

This comment has been minimized.

Copy link

commented on .travis.yml in dd153a6 Jun 14, 2019

No need for this one. The deployment below already allows manual deployment

notlmn added some commits Jun 14, 2019

Fix permissions forever
- Remove on tags for Travis build

- Rename option `newTabAlways` to `reuseTabs`

- Move permissions requests to options

- Remove some tests and respective dependencies

- Fix failing tests
@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

Published 0.0.0.2 to AMO for final review. The extension should now work similar in both Chrome and Firefox.

Requesting a final review from @sindresorhus and @bfred-it.

notlmn added some commits Jun 14, 2019

@notlmn notlmn requested a review from sindresorhus Jun 14, 2019

@ravanscafi

This comment has been minimized.

Copy link

commented Jun 14, 2019

Awesome! Notifications and clicking on the button on menu now are working!
Not sure about notification sound, though.

@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 14, 2019

Not sure about notification sound, though.

Yeah, was thinking about that too. I'm assuming it's probably something to do with autoplay blocking out of user interaction. I may be wrong, or someone can debug it for me.

@ravanscafi

This comment has been minimized.

Copy link

commented Jun 14, 2019

I do believe that it was working on the very beginning, when we had issues with browser.permissions.request().

@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 16, 2019

I do believe that it was working on the very beginning, when we had issues with browser.permissions.request().

I can manually verify that both notifications and notification sounds work fine.

@notlmn notlmn changed the title Setup publishing to AMO Setup publishing to AMO Jun 16, 2019

@notlmn notlmn merged commit f42c6f5 into master Jun 16, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@notlmn

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 16, 2019

🚀 Successfully published to AMO as 19.6.16.428, feel free to open new issues as they come.

@notlmn notlmn deleted the publish-amo branch Jun 16, 2019

@gizmecano gizmecano referenced this pull request Jun 16, 2019

Merged

Support Firefox #140

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.