fregante / notifications-preview-github

Browser Extension: preview GitHub notifications with same page pop-overs
https://chrome.google.com/webstore/detail/github-notifications-prev/kgilejfahkjidpaclkepbdoeioeohfmj?hl=en&gl=IN
MIT License
144 stars 15 forks source link

Add gist.github.com support #105

Closed darkred closed 4 years ago

darkred commented 4 years ago

Currently the extension is not working in gist.github.com pages, i.e. there's no notification number in the blue bubble nor pop-over on hover.

This PR adds https://gist.github.com/* in the extension's manifest.json. (I hope this change is all that's needed, because I couldn't get the loaded unpacked extension to work, I was getting errors)

Test URL: https://gist.github.com/discover

Thank you

tanmayrajani commented 4 years ago

Hi, so I checked and it seems it's not the only change needed (as you suspected as well)..

notice the following, it's using location.origin for building the URL to get notifications. Which doesn't work on gist.github.com. https://github.com/tanmayrajani/notifications-preview-github/blob/master/extension/github-notifications-preview.js#L16

darkred commented 4 years ago

Thank you for responding and for identifying that problem. Could you please tell me how to build the extension locally, in order to make sure that the extension works, before I add the commit to this PR ?

tanmayrajani commented 4 years ago

I noticed the same problem today for the local build but didn't have enough time at my hand to figure out and fix. If you can, see if you're able to fix that first. Maybe @fregante can help as well? As a last resort, you'll still be able to build and import the dist folder for now?

darkred commented 4 years ago

Maybe @fregante can help as well?

For reference, here is how I do Load Unpacked the "Extension" folder in Google Chrome, and the errors I get:

Then, if I try to Load Unpacked the "extension" folder again I now get Uncaught SyntaxError: Cannot use import statement outside a module at options-storage.js:1


As a last resort, you'll still be able to build and import the dist folder for now?

How do I do that? Which dist folder do you mean?


see if you're able to fix that first.

I'll try

fregante commented 4 years ago

The extension needs to be built first:

  1. Run npm install
  2. Run npm run build

Then loaded from the new dist folder that has been generated, not extension

I noticed the same problem today for the local build

It seems to work correctly for me and on GitHub Actions. You might just need to delete node_modules

darkred commented 4 years ago

Thank you very much @fregante . It works fine with your instructions (I also opened manifest.json and removed the "applications" key in the dist folder).


Now regarding the issue that @tanmayrajani kindly described: I can only think of changing in https://github.com/tanmayrajani/notifications-preview-github/blob/master/extension/github-notifications-preview.js#L16 the location.origin to fixed https://github.com but the extension still doesn't work: if I open e.g. https://gist.github.com/discover, I get this error Uncaught (in promise) TypeError: Failed to fetch :

the main error ![2020-08-03_234331](https://user-images.githubusercontent.com/723651/89225629-31eb0600-d5e3-11ea-95d9-d81f842f7ab9.jpg)

because of "Cross-Origin Read Blocking (CORB)":

2020-08-03_234657

which leads to Uncaught (in promise) TypeError: Cannot read property 'textContent' of null, i.e.:

the 2nd error ![2020-08-03_234050](https://user-images.githubusercontent.com/723651/89225414-d456b980-d5e2-11ea-83a3-b3422214c3c2.jpg)

 

Sorry I can't be of any more help.

fregante commented 4 years ago

I also opened manifest.json and removed the "applications" key in the dist folder).

That's not necessary. Chrome complains but it still works

because of "Cross-Origin Read Blocking (CORB)":

Ugh, that's an annoying problem and that probably means it's not possible.

In my npmhub extension we fixed that by moving the fetch to the background page (https://github.com/npmhub/npmhub/issues/92), however this is not possible here: we need the current page’s cookies and those are not available in the background page.

AFAIK, there are only 3 solutions:

Other than that, I don't see a solution, but feel free to keep looking for one.

fregante commented 4 years ago

I'll close this PR in the meanwhile, it's not enough