URL: Use test data from web-platform-tests for isURL spec conformance #20537
Conversation
aduth
added
[Type] Enhancement
|
Size Change: -1.34 kB (0%) Total Size: 864 kB
ℹ️ View Unchanged
|
|
@SergioEstevao Note the failing native tests. Is this something you'd want or be able to take on? (i.e. fixing the implementation) |
Early iterations included LICENSE fetching as string, but this was later changed to static file. As such, the utility is better served to include JSON parsing.
Interestingly, rejecting (throwing) does not itself halt execution.
|
@aduth I had a look and tried to implement a solution based on a REGEX, but I got to the conclusion that I was almost impossible to implement one that validates correctly all the cases on the test set. I was thinking couldn't we just import the full whatwg-url package in RN and use the URL class as we do in the web? @etoledom did you try to do that on your previous implementation? |
I had meant to circle back here before your comment. Coincidentally, I was going to suggest exactly the same. There was some discussion in #20172 about this. Apparently there were some issues in Android, but it's unclear if those were caused specifically by |
|
Based on @charpeni's comment at #20172 (comment), I tried again to reincorporate See log: https://travis-ci.com/WordPress/gutenberg/jobs/296580166 |
|
@aduth I will retry this on the simulators and see if all builds correctly now. I also think it's a good idea to add the unicode test to the exception lists for now. |
|
Some of the failures are a bit peculiar. For example: I guess it's based on the forked differences of I observe in the regular
I guess it may technically fall under the scope of "unicode" validation omitted by the fork? Aside: Does bundle size matter as much in React Native to warrant worrying so much over the ~100kb difference between Anyways, I guess I can make add some exception logic for now, since these are edge cases, even if technically valid. But the closer we can align these, the better. |
|
Tests related to Unicode support were skipped in charpeni/whatwg-url@473d32f. Tests are still running on CircleCI, and react-native-url-polyfill does have a Detox setup to run the polyfill on an iOS simulator.
A difference of ~341 KB*. 😬 Some context around this, It's worrying enough that it couldn't be included in the core of React Native, even with ~41 KB. Adding ~341 KB (+51%) to the bundle seems excessive to handle Unicode which are like 1% of URL use cases. |
Ah, thanks for correcting. I think I had the number stuck in my mind from the 133kb of Thanks for the extra context as well. I admit I have a tendency to obsess over the edge cases, but I think it's a case to consider being pragmatic, given the circumstances. |
|
@aduth I created a PR on GB-mobile project to add the polyfill library to the project again. It looks it's building correctly now, and all tests are green. Just waiting for @etoledom to double-check the build issues. |
for me!
Closes #20534
This pull request seeks to enhance tests for
@wordpress/urlisURLto conform to those defined by the Web Platform Tests project. As outlined in #20534, it includes a script to download and transform the URL test data, which is then loaded by the@wordpress/urlunit tests as fixture data for theisURLtests.You can run the script directly:
Testing Instructions:
Ensure unit tests pass: