fix(lottie.ts): passing credentials include param to fetch request #131
Conversation
There was a problem hiding this comment.
Thanks for the PR. I don't think this should be the default, but configurable. How about a named argument @fetchOptions so the call await fetch(this.args.path, this.args.fetchOptions) can be made? Would need a test, maybe sinon can be used, its already a dependency for the tests, to mock fetch and check options are passed correctly?
akil3
force-pushed
the
update-fetch-to-include-credentials
branch
from
e4fa109 to
c4c0627
Compare
…request Fetch request is failing with 403 when trying to fetch lottie json hosted on another domain that gates the assests behind credentials. https://javascript.info/fetch-crossorigin#credentials
akil3
force-pushed
the
update-fetch-to-include-credentials
branch
from
c4c0627 to
f9cc6be
Compare
Also removed ember-window-mock since its really not needed and all tests pass w/out it
|
@mrloop added |
… causing incompatibility with sinon is fixed
…it work with ember-window-mock
pnpm-lock.yaml
Outdated
| @@ -1,4 +1,8 @@ | |||
| lockfileVersion: '6.0' | |||
| lockfileVersion: '6.1' | |||
There was a problem hiding this comment.
Looks like your using pnpm v8.6.1, there was breaking change with this release which was fixed in v8.6.2. See pnpm/pnpm#6648. If you upgrade pnpm and then upgrade ember-window-mock again then the lock file wont change it's lock file version. We want to remain on '6.0'. Everything else looks good
There was a problem hiding this comment.
Upgraded pnpm to v8.6.2 and updated the package.
The version is back to 6.0 but its still adding these properties for me though.
settings: autoInstallPeers: true excludeLinksFromLockfile: false
There was a problem hiding this comment.
OK, I'd remove those settings. Also the build is failing for unrelated issues, it should pass if you rebase on main, I spent a bit of time yesterday merging outstanding PRs which fixed the failing build. Ideally this PR would be in two commits the first commit with the dependency update and the second with the feature update. Alternatively one commit would be fine, with all of them squashed. This means we'll have a clean history, and one which all commit should build and pass test suite, helpful if you ever need to git bisect the repo.
There was a problem hiding this comment.
Quick update, do want merge this PR, but ember-window-mock is a breaking change due to it dropping support for ember 4.4. So in another PR me or another maintainer will drop 4.4 and other unsupported ember versions. 4.4 last security updates are in a months time. The PR is also failing on other ember version but think its a CI issue, so I need to fix the build pipeline. Not sure of the timeline
There was a problem hiding this comment.
Thanks for the update @mrloop
|
I've rebased and cleaned up in #274, closing this MR in favour of that one |
Fetch request is failing with 403 when trying to fetch lottie json hosted on another domain that gates the assests behind credentials. https://javascript.info/fetch-crossorigin#credentials