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

feat: parser refactor and updates #505

Open
wants to merge 38 commits into
base: feat/the-grid
Choose a base branch
from

Conversation

nastita
Copy link
Collaborator

@nastita nastita commented Oct 18, 2024

Ticket ID

DEV-11851
DEV-11989

Description

  • Refactored input parser to streamline flow for all platforms
  • Added better twitter input parsing support

Enhancements for other platforms will come in follow up prs

@nastita nastita marked this pull request as ready for review October 21, 2024 19:37
@nastita nastita changed the title WIP parser refactor and updates feat: parser refactor and updates Oct 21, 2024
@Hugoo
Copy link
Contributor

Hugoo commented Oct 21, 2024

Copy link
Contributor

github-actions bot commented Oct 21, 2024

Deployed with Cloudflare Pages ☁️ 🚀 🆗

@nastita nastita requested a review from dzbo October 22, 2024 12:20
@nastita nastita self-assigned this Oct 22, 2024
Copy link
Collaborator

@dzbo dzbo left a comment

Choose a reason for hiding this comment

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

Let's merge this for now so we have oembed in use. Ideally we should break down parsers into individual platform files for easier customization.

@dzbo
Copy link
Collaborator

dzbo commented Oct 31, 2024

Seem like it doesn't work for just a profile name:

due to cors policy kick in, no-cors doesn't help much as it resolve in Unexpected end of input for opaque response.
CleanShot 2024-10-31 at 00 47 08@2x

Non browser request works fine:
CleanShot 2024-10-31 at 01 43 37@2x

I wonder if we could proxy this through our API, thoughts @richtera ?

Also, for X we can skip oembed as it's straight forward to parse their embed. Should we just use oembed when we really need?

@Hugoo
Copy link
Contributor

Hugoo commented Nov 1, 2024

Task linked: DEV-11989 Prevent SoundCloud autoplay

matches = Array.from(input.matchAll(regex))
} else {
const match = input.match(regex)
console.log(JSON.stringify(match))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's not use console.log, we have own logging mechanism:

if (gridLog.enabled) {
  gridLog(JSON.stringify(match))
}

domains/grid/shared/config.ts Outdated Show resolved Hide resolved
domains/schema/xWidgetSchema.ts Outdated Show resolved Hide resolved
@nastita nastita requested a review from dzbo November 4, 2024 16:23
regexWithCallbacks: [
// Match whole embed code
{
regex: createIframeRegex('https:\\/\\/warpcast\\.com\\/(?<id>[\\w.-]+)'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally it should be ```console

  • 16 characters or fewer
  • Latin alphabet, numbers or dash (-)
  • Cannot start with a dash
  • Must end in (.eth), i.e. ENS-compatible names like [cb.id](http://cb.id/) aren’t supported at this time
  • Not supported at this time:
    • subdomains, e.g. abc.test.eth
    • DNS to ENS
So maybe something like `\\/(?<id>[a-z0-9][a-z0-9-.]{0,15}$)`. They don't actually support unicode or other chars. Only a-z0-9 and - it seems. Although the "." in the name is not in the offficial docs although they say that ENS names work and they obviously require a dot. I suppose invalid names would just end up with an error page and should not be that bad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants