-
Notifications
You must be signed in to change notification settings - Fork 152
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
Commented out share_target implementation #383
Conversation
The share_target implementation is breaking the build. This PR fixes what is breaking it. We also set the shareTarget variable to `undefined`, effectivelly disabling the feature. The reason beint that no validation is being done in the value read from the Web Manifest, which can potentlially generate an invalid TWA Manifest.
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.
I'd prefer if we got rid of the commented out code, but if we're in a rush, LGTM.
packages/core/src/lib/TwaManifest.ts
Outdated
@@ -297,7 +297,7 @@ export class TwaManifest { | |||
features: { | |||
locationDelegation: {enabled: DEFAULT_ENABLE_LOCATION}, | |||
}, | |||
shareTargetJson: JSON.stringify(webManifest['share_target']), | |||
shareTarget: undefined, // webManifest['share_target'], |
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.
I don't really like checking in commented out code. Potentially what we could have is a comment linking to a bug or issue explaining why it's undefined.
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.
Removed the commented-out code and added a comment. I want to avoid having broken things in master
.
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.
LGTM
The share_target implementation is breaking the build. This PR
fixes what is breaking it.
We also set the shareTarget variable to
undefined
, effectivelydisabling the feature. The reason being that no validation is
being done in the value read from the Web Manifest, which can
potentially generate an invalid TWA Manifest.
Related to #21