-
Notifications
You must be signed in to change notification settings - Fork 25
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
Move instagram error handling to link level #132
Conversation
@@ -7,6 +7,14 @@ const getProviderEndpointForLinkUrl = (linkUrl, providers) => { | |||
schema = schema.replace("*", ".*"); | |||
const regExp = new RegExp(schema); | |||
if (regExp.test(linkUrl)) { | |||
if (provider.provider_name === "Instagram") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor (blocking): Move the error check into the providers loop so that we can throw the error as soon as we're on the Instagram provider.
refactor (non-blocking): These do not need to be two if
blocks. It can be one with a consolidated condition.
refactor (future PR/non-blocking/suggestion): The code in this function has three nested loops, so it's O(n3) (cubic time). We can probably improve upon this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did the first two, will break the third out as an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refactor (blocking): Move the error check into the providers loop so that we can throw the error as soon as we're on the Instagram provider.
Just realized this will not give us the intended behaviour. We need to check if an oembed link is in fact an Instagram link by checking against the schemas. If there are no instagram links, no need to let anyone know there an access token needs to be configured. Will make a test case for this, so we remember in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great @raae. a few comments/request changes.
gatsby-remark-oembed/utils/getProviderEndpointForLinkUrl.test.js
Outdated
Show resolved
Hide resolved
Thank you for great suggestions, I will do them all :D |
Co-authored-by: Nick Taylor <nick@iamdeveloper.com>
d2eb0c0
to
6b318ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great @raae! 🚢
🎉 This PR is included in version 0.2.0-next.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 0.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
The core design of the plugin is that its very forgiving. If something cannot be embedded we display the link as it. It should not stop the build if there are errors with individual embed links.
Related issues
Fun