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

fix(entity-link): more robust url regex #933

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

mihai-peteu
Copy link
Contributor

Summary

URL regex should allow dev preview URLs / .tech / .com domains, but alsolocalhost with http protocol, followed

PR Checklist

  • Naming & Structure: the files and package structure use the conventions outlined in the Creating a Package docs.
  • Tests pass: check the output of all package unit and/or component tests.
    • If this PR is the result of a bug, test coverage was added accordingly.
  • Functional: all changes do not break existing APIs, but if so, a BREAKING CHANGE commit is in place to bump the major version.
  • Conventional Commits all commits follow the conventional commit standards outlined in the main README.
  • Docs: includes a technically accurate README, and the docs have been updated accordingly based on the changes in this PR.

@mihai-peteu mihai-peteu requested review from a team as code owners November 20, 2023 21:27
@mihai-peteu mihai-peteu changed the title Fix/ma 2278 entity link url regex fix(entity-link): more robust url regex Nov 20, 2023
const urlRegex = /https?:\/\/(www\.)?[-a-zA-Z0-9@:%._\+~#=]{2,256}\.[a-z]{2,4}\b([-a-zA-Z0-9@:%_\+.~#?&//=]*)/g // eslint-disable-line no-useless-escape
// Test validity of generated URL; only allows `http` protocal if domain is `localhost`
/* eslint-disable-next-line no-useless-escape */
const validUrlRegex = /^(https:\/\/(www\.)?([a-zA-Z0-9-]+\.){1,}[a-zA-Z]{2,}(:[0-9]+)?(\/[^\/]+)*)$|^(https|http):\/\/localhost(:[0-9]+)?(\/[^\/]+)*$/
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to add a unit test for this composable to test happy and bad paths?

Copy link
Contributor Author

@mihai-peteu mihai-peteu Nov 21, 2023

Choose a reason for hiding this comment

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

Good call - pushed a commit with a few more passing + failing cases

@mihai-peteu mihai-peteu enabled auto-merge (squash) November 21, 2023 18:29
Copy link
Member

@adamdehaven adamdehaven left a comment

Choose a reason for hiding this comment

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

lgtm, defer to @hfukada for final

@mihai-peteu mihai-peteu merged commit 4aaee15 into main Nov 21, 2023
@mihai-peteu mihai-peteu deleted the fix/ma-2278-entity-link-url-regex branch November 21, 2023 19:09
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.

3 participants