fix(core): bundle react-native-url-polyfill to unblock jest test failure #11411
Conversation
AllanZhengYP
force-pushed
the
v5/custom-clients-bundle-polyfill
branch
from
77472db to
000aaa4
Compare
AllanZhengYP
force-pushed
the
v5/custom-clients-bundle-polyfill
branch
from
000aaa4 to
0e1b9d7
Compare
| @@ -1633,6 +1633,7 @@ releasable_branches: &releasable_branches | |||
| - main | |||
| - next | |||
| - v5/custom-clients | |||
| - v5/custom-clients-bundle-polyfill | |||
There was a problem hiding this comment.
Can this be removed before merging the PR?
| @@ -30,6 +30,7 @@ | |||
| "publish:release": "lerna publish --conventional-commits --yes --message 'chore(release): Publish [ci skip]' --no-verify-access", | |||
| "publish:verdaccio": "lerna publish --no-push --canary minor --dist-tag=unstable --preid=unstable --exact --force-publish --yes --no-verify-access", | |||
| "publish:v5/custom-clients": "lerna publish --canary --force-publish \"*\" --yes --dist-tag=v5-custom-clients --preid=v5-custom-clients${PREID_HASH_SUFFIX} --exact --no-verify-access", | |||
| "publish:v5/custom-clients-bundle-polyfill": "npm run publish:v5/custom-clients", | |||
There was a problem hiding this comment.
Can this be removed before merging the PR?
There was a problem hiding this comment.
yes, it was removed previously. Adding it back just to make sure the PR-fixing commits also pass the integ test
| @@ -1,7 +1,7 @@ | |||
| // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| import 'react-native-url-polyfill/auto'; | |||
| import '@aws-amplify/core/polyfills/URL'; | |||
There was a problem hiding this comment.
Does relative path not work for this?
There was a problem hiding this comment.
Relative path works too, but I used that one for easier location, because it's not from the src root path like all other folders. And the reason the polyfill is not put in source is because the folder should NOT be transpiled by TSC. It uses it's own webpack config.
| * is not supported by default. | ||
| */ | ||
| if (process?.env?.NODE_ENV !== 'test') { | ||
| // Loading this polyfill in customers' unit tests will cause undefined |
There was a problem hiding this comment.
Nit: Is it only in customers' unit tests?
…ill" This reverts commit dbacebc.
AllanZhengYP
force-pushed
the
v5/custom-clients-bundle-polyfill
branch
from
0e1b9d7 to
01c3c15
Compare
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## v5/custom-clients #11411 +/- ##
==================================================
Coverage 83.23% 83.23%
==================================================
Files 274 274
Lines 20522 20522
Branches 4437 4437
==================================================
Hits 17082 17082
Misses 3152 3152
Partials 288 288 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Replaced by #11422 |
Description of changes
This change bundles the
react-native-url-polyfillpackage(excluding transitive dependencies), into a commonjs module, and referred from the ReactNative entrypoint.Issue #, if available
The additional RN polyfill for URL would break consumer's ReactNative unit test with Jest. See this error. This is because this polyfill package uses MJS module only. Although the ReactNative's bundler would cope with it, Jest does not support MJS out-of-box. This is considered as breaking change by breaking customer's unit tests.
Description of how you validated changes
Unit test; Integration test(Amplify JS & UI); Manual RN test.
TODO:
This change is no longer necessary if the upstream dependency issure is resolved: charpeni/react-native-url-polyfill#446. It's planned to be done after the release.
Checklist
yarn testpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.