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

Add support for GitHub Enterprise #450

Merged
merged 8 commits into from
Jun 1, 2017
Merged

Add support for GitHub Enterprise #450

merged 8 commits into from
Jun 1, 2017

Conversation

fregante
Copy link
Member

@fregante fregante commented May 26, 2017

Fixes #315

New domains can be specified in GH's options view, which can also help #193, should that ever materialize.

options view

To test this without access to GHE:

  1. remove all the permissions to github.com in manifest.json
  2. disable and re-enable the extension (don't just reload it)
  3. Verify the permissions, github.com should not be there
  4. Add https://github.com via the form

@RodEsp
Copy link

RodEsp commented May 26, 2017

@bfred-it this is gold.
@sindresorhus could we get some eyes on this? I'd love to have it merged as soon as possible.

@busches
Copy link
Member

busches commented May 26, 2017

Should there be an option to see currently added domains and remove them if no longer needed?

@fregante
Copy link
Member Author

@busches I don't think it's a big deal, worst thing that can happen is that a domain you no longer use takes 20 bytes on disk, I think.

@fregante
Copy link
Member Author

fregante commented May 27, 2017

Alternative style:

@derimagia
Copy link

derimagia commented May 29, 2017

@bfred-it Very nice work! Tested this on GHE and the actual permission stuff worked great. All of the css changes worked without issue.

However the javascript doesn't fire at all because this is loading after the page loaded and the javascript changes are done using a listen on the DOMContentLoaded event and not taking into account if it's loaded after that event has triggered. Should be easy to fix. content.js should probably be executed last (right now there are a few after it and just calling the content loaded function errors because of this)

It also looks like right now you're guaranteeing execution order - the javascript is noticeably slower to load on the GHE sites as well and this may be why. We should be able to load all of them async except for content.js and load that after, right? I'm not sure if this is why we are firing after the dom loaded event or not, but either way it would improve speed of load.

@fregante
Copy link
Member Author

fregante commented May 29, 2017

Thanks for the info! I'll apply the two fixes (domready and content.js loaded last) but the speed will automatically be improved once #404 is in and only a file is loaded.

I'll also try removing the order guarantee, perhaps Chrome already does this (premature optimization?)

@fregante fregante changed the title Add support for custom domains Add support for GitHub Enterprise May 30, 2017
@paulmolluzzo
Copy link
Contributor

I don't use GHE, so this PR is out of my reach I think. @hkdobrev maybe? Or @jgierer12?

fregante added a commit to fregante/webext-dynamic-content-scripts that referenced this pull request May 30, 2017
refined-github/refined-github#450 (comment)
724281

Chrome seems to already run scripts in order. Tested with
file 1. ember + three.js + console.log(1)
file 2. console.log(2)
i.e. GitHub Enterprise
* Only inject the scripts when permitted
* Move potentially slow operation to the last moment
@fregante
Copy link
Member Author

fregante commented May 30, 2017

↕️ Applied the 3 suggested changes and rebased.

@jgierer12
Copy link
Contributor

Nope, I don't use GHE either

@derimagia
Copy link

@bfred-it Got it

Confirming this is working for me on GHE. Also note you can see what sites you gave permission to in the settings for the extension.

@fregante
Copy link
Member Author

Thanks @derimagia! Hopefully this can be merged/squashed soon :)

@@ -1,5 +1,5 @@
<!DOCTYPE html>
<meta charset="UTF-8">
<!doctype html>
Copy link
Member Author

Choose a reason for hiding this comment

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

It's case-insensitive, but the official one is actually uppercase: https://html.spec.whatwg.org/multipage/syntax.html#the-doctype

<!DOCTYPE html>
<meta charset="UTF-8">
<!doctype html>
<meta charset="utf-8">
Copy link
Member Author

Choose a reason for hiding this comment

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

@busches
Copy link
Member

busches commented May 31, 2017

This works well for the most part. There are still a lot of places that have github.com hardcoded, such as branch names, reaction avatars, show names, etc. These links do not work when you're on another domain as they incorrectly go to the wrong URL. This is a larger undertaking, so I can see it being moved out to another PR, but is core to supporting other domains.

@fregante
Copy link
Member Author

@busches do you see value in merging this now to get at least some features working while further and specific issues can be reported and fixed later?

@busches
Copy link
Member

busches commented May 31, 2017

@bfred-it yes. We'd just need to create an issue for what I mentioned above.

@fregante
Copy link
Member Author

fregante commented May 31, 2017

Cool. There's a chance that some features won't ever be available or compatible with GHE, but custom domain support at least gives a basis for other to find and fix issues too, especially because I don't actually have access to any GHE.

@derimagia
Copy link

@bfred-it I'd be happy to work with you as needed, what features aren't compatible with it would you say?

@busches
Copy link
Member

busches commented May 31, 2017

@derimagia anything that has github.com hardcoded, instead of using something like location.origin:

  • Linkify Branch References
  • Add Delete Fork Link
  • isGist function detection
  • Reaction Avatar images
  • Showing Real Name next to username
  • Linkify issues

There could be more, but from a quick glance at the code and attempting them, those don't work.

@derimagia
Copy link

Sorry I mean't this part of it: "Some features won't ever be available or compatible with GHE" - is it that they can't be done or that we would need to fix the hard code locations?

@fregante
Copy link
Member Author

fregante commented May 31, 2017

I don't have GHE so I'm only guessing of other possible issues, considering that GHE is not 1:1 with GH

The URLs might only need to be changed from https://github.com/something to /something, like d9c7d99. But that can come in a different PR and further testing.

@sindresorhus
Copy link
Member

@busches Can you open an issue about further improvements to GHE support?

@sindresorhus sindresorhus merged commit 1bd2494 into refined-github:master Jun 1, 2017
@fregante fregante deleted the enterprise branch June 1, 2017 16:40
@sindresorhus
Copy link
Member

@bfred-it Can you mention GHE support in the readme?

@hkdobrev hkdobrev added enterprise Related to GitHub Enterprise only enhancement labels Jun 1, 2017
@jgierer12
Copy link
Contributor

Just a heads-up, this doesn't work on Firefox since it doesn't allow changing permissions dynamically. I don't think there's much we can do about it currently, so maybe mention this in the readme too @bfred-it

@fregante
Copy link
Member Author

fregante commented Jun 2, 2017

Pinging some people who showed interest in Refined GitHub Enterprise:

Would you mind giving it a go and reporting new issues?

@zafarella
Copy link

Thank you so much for adding the support. I was adding the GHE URL to manifest file as workaround.

Works for me but only after installing it from store. When loading locally, it does not work.
Extension was this red box

There were warnings when trying to install this extension:
Unrecognized manifest key 'applications'.

I pulled latest master and:

npm  install
npm run build

@fregante
Copy link
Member Author

fregante commented Jul 6, 2017

That's just Chrome being annoying. You can ignore that warning, it works anyway, you have to authorize the domain again in the options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement enterprise Related to GitHub Enterprise only
Development

Successfully merging this pull request may close these issues.

None yet

9 participants