[Mobile] Use react-native-url-polyfill in globals#27867
Merged
Conversation
2 tasks
|
Size Change: 0 B Total Size: 1.3 MB ℹ️ View Unchanged
|
6 tasks
Contributor
|
Change looks good to me, could we have a test as part of this change as well. If needed I can test that removing the import actually makes the test fail |
Contributor
Author
It turns out we already have several tests for url, which were passing spuriously. In a previous approach (before I realized we are already including this polyfill with our bundle, but not using it universally), we tried manually patching the This also explains why the unit tests did not break, even though the actual behavior in the app did break. I created this branch to demostrate that several |
Contributor
|
This works as expected for me. Lets fix the conflict in the index.native.js file and ship this :) |
This platform fork is no longer necessary, since we are using the polyfill globally.
Contributor
Author
|
It seems the fixture tests for native were disabled when the native test file was deleted here. I've re-added the tests, and ensured we are testing against the polyfill implementation used in production with this PR. I've added some exclusions to the fixtures, since they've been updated in the same jest upgrade PR. Also, I removed the native fork of |
enejb
approved these changes
Jan 6, 2021
Contributor
enejb
left a comment
There was a problem hiding this comment.
I tested this and it worked well for me. Thanks for the fixes!
Contributor
Author
|
Thanks for the review Enej, and for troubleshooting the CI issues with me. 😄 |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related PR
gutenberg-mobile: wordpress-mobile/gutenberg-mobile#2923Description
Uses
react-native-url-polyfillvia import inglobals.js. This is needed for properly merging URL query parameters duing middleware processing for endpoints utilising api-fetch. We have already incurred the cost to the bundle size, since we are utilizing this module inis-url.native.js. This PR makes sure we are using the polyfill implementation globally.It seems that the unit tests we currently have for URL are not being run against the default URL implementation provided by the react-native runtime. I've opened a branch (#27868) for demonstration purposes to have the tests running with the default URL implementation, and several of the tests fail. I've also added logging for errors, which typically are caught and silently ignored. We can see here the failures arise from unimplemented getters.
This PR fixes this issue: wordpress-mobile/gutenberg-mobile#2922
Steps to test
Expected behavior
The results should be different for searches which have differerent queries. The search results should reflect the query being searched for.
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: