Skip to content

strip trailing slashes for go links#5023

Merged
patrickkettner merged 3 commits intofuturefrom
no-trailing-slash-for-go
Dec 5, 2020
Merged

strip trailing slashes for go links#5023
patrickkettner merged 3 commits intofuturefrom
no-trailing-slash-for-go

Conversation

@patrickkettner
Copy link
Copy Markdown
Collaborator

fixes #3859

@sebastianbenz
Copy link
Copy Markdown
Collaborator

Can you please add a test in go.test.js.

@patrickkettner
Copy link
Copy Markdown
Collaborator Author

jawoll captain

@patrickkettner patrickkettner force-pushed the no-trailing-slash-for-go branch from 6af1d1d to a44f546 Compare December 4, 2020 18:07
@patrickkettner patrickkettner force-pushed the no-trailing-slash-for-go branch from a44f546 to c516aca Compare December 4, 2020 18:08
});
} else {
simple[key] = value.url;
simple[`${key}/`] = value.url;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

what if the key already has a trailing slash?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

These are static strings, it would be /foo/ and /foo//. Handling it felt like the complexity outweighed the use since there are no uses of it currently. It would allow for /foo/ and /foo//.

That being said, I do something like

} else {
  key =  key.replace(/\/?$/, '/?');
  simple[key] = value.url;
}

which would force an optional trailing slash (e.g. /?) on all simple routes

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think it'd be good to consistently support both schemes with and without trailing slash. Stripping trailing slashes when loading the input and when handling a request (like in your first version) might be the easiest way to do this.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think it'd be good to consistently support both schemes with and without trailing slash.

makes sense

pushed an update, wdyt

Copy link
Copy Markdown
Collaborator

@sebastianbenz sebastianbenz left a comment

Choose a reason for hiding this comment

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

Looks good once build passes. Thanks!

@patrickkettner patrickkettner merged commit c9f8008 into future Dec 5, 2020
@patrickkettner patrickkettner deleted the no-trailing-slash-for-go branch December 5, 2020 01:36
robinvanopstal added a commit that referenced this pull request Dec 6, 2020
* future:
  strip trailing slashes for go links (#5023)
  fix invalid html in playground demos (#5026)
  Add email sender guides (#4948)
  Update dependency highlight.js to v10.4.1 (#5016)
  Update dependency mini-css-extract-plugin to v1.3.2 (#5021)
  typo fixes (#5019)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

go.amp.dev links don't support trailing slash

2 participants