Script Loader #1
Conversation
this will allow easy merging of squash commits to inform versioning
should prevent class of errors where developers try to use sandbox application id with production script src and vice versa
src/load.ts
Outdated
| } | ||
|
|
||
| function injectScript(src: string): HTMLScriptElement { | ||
| const script = document.createElement('script'); |
There was a problem hiding this comment.
nit: create the script element after we know there is a head or body rather than before
| } | ||
|
|
||
| if (window.Square) { | ||
| resolve(window.Square); |
There was a problem hiding this comment.
do we want to verify this namespace is valid/ours first?
There was a problem hiding this comment.
we could be more explicit, but I'm not exactly sure how to right away (early days). currently, it'll be implicitly validated when payments is called on it.
| ? overrides.scriptSrc | ||
| : getSrcForApplicationId(applicationId); | ||
|
|
||
| const maybeSquare = await loadSquare(src); |
There was a problem hiding this comment.
the next step is to create some examples to better understand how this will be used which should hopefully inform what's useful to handle versus let bubble up. currently, I believe only bad script loads would cause rejections.
| @@ -0,0 +1,18 @@ | |||
| name: Semantic Release | |||
There was a problem hiding this comment.
Do we expect the semver version of our npm module to relate in any way to the WebSDK versions?
There was a problem hiding this comment.
I expect this module to respect the semver of websdk but not move in lock step. for example, if websdk releases v42 then this module would create a major release changing the version constant in the loader. if a breaking change is needed here, it can release a major bump independently.
| @@ -21,6 +21,7 @@ | |||
| "babel-jest": "^26.6.3", | |||
| "conventional-changelog-conventionalcommits": "^4.5.0", | |||
| "eslint": "^7.19.0", | |||
| "eslint-plugin-jest": "^24.1.5", | |||
There was a problem hiding this comment.
Should we prefer yarn instead of npm give that this is what we use in the WebSDK?
There was a problem hiding this comment.
I'm starting w/ npm since v7 filled a lot of the gaps yarn v1 previously filled. I'm hoping it'll also set up all the docs, examples, and CI scripts to be pure npm for versioning, publishing, etc. (you can use yarn out of habit and may not even notice)
| @@ -0,0 +1,11 @@ | |||
| interface Payments { | |||
| verifyBuyer: () => void; | |||
There was a problem hiding this comment.
// TODO: Add all of our typings? :)
| } | ||
|
|
||
| try { | ||
| let script = findScript(src); |
There was a problem hiding this comment.
I'm trying to understand the reason why a script might already be present given we're caching the promise value. Is this an edge case, or something we expect to happen often?
There was a problem hiding this comment.
definite edge case in an attempt not to double-load by accident
| src = 'https://sandbox.websdk.squarecdn.com/'; | ||
| } | ||
|
|
||
| if (src.length === 0) { |
There was a problem hiding this comment.
Worth a reminder here that this assumption is somewhat flawed. There are application IDs in the wild that don't use the prefixes.
|
🎉 This PR is included in version 1.0.0-beta.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Initial functionality of validating credentials and loading the correct script