Add support for base URL #24

g12i · 2020-08-05T17:06:16Z

Solves #23

I used 2 url functions, both added in Node.js v0.1.25

url.resolve - safer way to resolve base + path
url.parse - see if hostname is present in whatever repostiory is set to

I had to update all the test cases that mentioned https://codeload.github.com/, by the new rules they resolve to https://codeload.github.com/

g12i · 2020-08-05T17:08:40Z

lib/util/resolve-base-url.js

+  var parsed = url.parse(repository)
+
+  if (parsed.host) {
+    return 'https://' + parsed.host + '/'


I can't use parsed.protocol as it resolves to original protocol (e.g. git://), which is not what we need. But then if someone hosts GHE on http:// this will lead to errors :(

Are they always https?

Does d = new URL(url || 'https://github.com'); d.protocol = 'https'; return d.hostname work?

I'm afraid it has to be hardcoded in this fashion.

The WHATWG URL Standard considers a handful of URL protocol schemes to be special in terms of how they are parsed and serialized. When a URL is parsed using one of these special protocols, the url.protocol property may be changed to another special protocol but cannot be changed to a non-special protocol, and vice versa.

Likewise, changing from a non-special protocol to a special protocol is also not permitted:

const u = new URL('fish://example.org'); u.protocol = 'http'; console.log(u.href); // fish://example.org

(source)

lol. That’s...

Can we work around it by doing something like: new URL('http:' + url.href.slice(url.href.indexOf(':')))?

I love url, but it’s a bit of a downside that it’s deprecated and the suggestion is to use URL

Switched it back to concatenation. Alternatively I can use:

return new URL('/' + repo.user + '/' + repo.project, base) + '/'

which would be more foolproof, in case someone provide baseUrl with trailing slash /, leading to https://github.com//such/urls

package.json

wooorm

Nice, sorry for the wait!

Some suggestions inline, and could use docs!

package.json

test/index.js

wooorm · 2020-08-23T10:18:53Z

lib/index.js

@@ -39,23 +40,30 @@ var repositoryExpression = new RegExp(
 function github(options) {
  var settings = options || {}
  var repository = settings.repository
+  var baseUrl = settings.baseUrl


how about just base? (and githubBase too?)

Personally I think "base" is too generic, when providing plugin options. But if you insist I'm happy to change it.

lib/util/gh.js

wooorm · 2020-08-23T10:21:51Z

lib/util/resolve-base-url.js

+  var parsed = url.parse(repository)
+
+  if (parsed.host) {
+    return 'https://' + parsed.host + '/'


Are they always https?

Does d = new URL(url || 'https://github.com'); d.protocol = 'https'; return d.hostname work?

g12i · 2020-09-08T15:37:12Z

Hey @wooorm! Sorry for wait, I was on holiday :). I switch to the modern API and removed unnecessary fixtures. Take a look, while I'll work on documentation.

wooorm

Fancy! A couple code style suggestions.

And hope you had a lovely holiday!

wooorm · 2020-09-08T16:00:53Z

lib/util/gh.js

@@ -2,18 +2,18 @@

 module.exports = gh

+const {URL} = require('url')


drop this one, it isn’t really needed in this file

wooorm · 2020-09-08T16:01:44Z

lib/util/gh.js

  if (project) {
    repo = {user: repo, project: project}
  }

  if (repo) {
-    base += repo.user + '/' + repo.project + '/'
+    return new URL('/' + repo.user + '/' + repo.project, base) + '/'


Suggested change

return new URL('/' + repo.user + '/' + repo.project, base) + '/'

return base + '/' + repo.user + '/' + repo.project + '/

You sure? Have you seen my comment about trailing slashes?

Sorry I missed this one!

I dunno, I feel that that’s a failure by users and should result in an error / that output?

lib/util/resolve-base-url.js

Co-authored-by: Titus <tituswormer@gmail.com>

into base-url-support

wooorm · 2020-10-05T19:12:57Z

Oh gosh, I forgot about this WIP issue/PR when I rewrote the project. 😓 What do you think about my last comment?

g12i · 2020-10-06T17:15:44Z

Oh snap! I forgot about this as well :D. I'll try to find some time and open the new PR. It would be easier than resolving conflicts here.

wooorm · 2020-10-06T17:24:02Z

The codebase is significantly easier now, and the changes here are roughly good to go, so applying them to the new code should be possible and easier than merge conflicts!

I don’t have access to enterprise so I don‘t feel comfortable doing most of the work, but I can help you!

Add support for base URL

144ffa9

g12i mentioned this pull request Aug 5, 2020

Support custom GitHub url #23

Closed

g12i commented Aug 5, 2020

View reviewed changes

g12i added 2 commits August 5, 2020 19:10

Fix test description

dc80be3

Do not fail build when using deprecated API

0a68725

g12i commented Aug 5, 2020

View reviewed changes

package.json Outdated Show resolved Hide resolved

Cover missing branch

eb5f2b9

This comment has been minimized.

Sign in to view

wooorm reviewed Aug 23, 2020

View reviewed changes

g12i added 2 commits September 8, 2020 17:32

Use modern APIs

1c02927

Remove fixtures, replace 443 with 8080

599a98e

g12i added 2 commits September 8, 2020 17:52

Update README

fcc14c1

Make URL resolving safe for trailing slashes

99d6cef

wooorm reviewed Sep 8, 2020

View reviewed changes

g12i and others added 3 commits September 8, 2020 19:44

Apply suggestions from code review

8dd3214

Co-authored-by: Titus <tituswormer@gmail.com>

Prettierify README

0536dda

Merge branch 'base-url-support' of https://github.com/g12i/remark-github

d62c381

into base-url-support

g12i closed this Oct 6, 2020

EvHaus mentioned this pull request Oct 11, 2021

Add buildUrl option for GH enterprise, custom URLs #31

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for base URL #24

Add support for base URL #24

g12i commented Aug 5, 2020

g12i Aug 5, 2020 •

edited

wooorm Aug 23, 2020

g12i Sep 8, 2020

wooorm Sep 8, 2020

g12i Sep 8, 2020

This comment has been minimized.

wooorm left a comment

wooorm Aug 23, 2020

g12i Sep 8, 2020

wooorm Aug 23, 2020

g12i commented Sep 8, 2020

wooorm left a comment

wooorm Sep 8, 2020

wooorm Sep 8, 2020

g12i Sep 8, 2020

wooorm Oct 3, 2020

wooorm commented Oct 5, 2020

g12i commented Oct 6, 2020 •

edited

wooorm commented Oct 6, 2020

		@@ -2,18 +2,18 @@

		module.exports = gh

		const {URL} = require('url')

	return new URL('/' + repo.user + '/' + repo.project, base) + '/'
	return base + '/' + repo.user + '/' + repo.project + '/

Add support for base URL #24

Add support for base URL #24

Conversation

g12i commented Aug 5, 2020

g12i Aug 5, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

wooorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g12i commented Sep 8, 2020

wooorm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wooorm commented Oct 5, 2020

g12i commented Oct 6, 2020 • edited

wooorm commented Oct 6, 2020

g12i Aug 5, 2020 •

edited

g12i commented Oct 6, 2020 •

edited