Flesh out the URL polyfill a bit more #22901
Conversation
…ct Native contexts. Add explicit-fail getters so undefined values won't get generated from the otherwise missing implemenation.
facebook-github-bot
added
CLA Signed
Generated by 🚫 dangerJS |
…w does not support. flow isn't enabled in the in-tree whatwg-fetch either, which also uses Symbol.iterator.
| @@ -5,7 +5,6 @@ | |||
| * LICENSE file in the root directory of this source tree. | |||
| * | |||
| * @format | |||
| * @flow strict-local | |||
There was a problem hiding this comment.
It'll be better not to disable flow
There was a problem hiding this comment.
I could not find anyone who got flow working with Symbol.iterator(), and only found issues where flow maintainers said the necessary annotation feature would not be added to flow. The whatwg-fetch implementation we have in-tree also doesn't have flow annotation, so I opted to fall in line with that. Let me know if it's a showstopper.
There was a problem hiding this comment.
can we add a flowfixme in that place? RN's flow types are already incomplete, so it'll be nice not to introduce more untyped code. anyway, it's upto the reviewer from fb
The whatwg-fetch implementation we have in-tree also doesn't have flow annotation
i think that was just vendored from the original library, so it's a different case
| } | ||
| } | ||
|
|
||
| export class URL { |
There was a problem hiding this comment.
From a quick look, the codebase doesn't seem to use ES6 export. Maybe do exports.URL = URL instead to keep it consistent
There was a problem hiding this comment.
I changed it due to a flow warning about mixed exports from before I disabled flow. Is there a reason ES6 export wasn't part of the general sweep to convert things to ES6 classes, etc?
There was a problem hiding this comment.
I don't know the reason. maybe someone from FB can clarify. but will be nice to keep it consistent with other code regardless of the reason
…ring static methods to top of class. Change XML quoting back to original.
|
Fixes #22594 |
|
Apart from the Flow and ES6 import comment, looks good to me. |
facebook-github-bot
added
the
Import Started
There was a problem hiding this comment.
@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
This looks fine for me for now. I wonder if there is a third-party dep for URL we could use instead of maintaining our own copy? |
|
@cpojer We've been using url-polyfill in our RN app for some time. We don't use the |
|
@matthargett merged commit 6303850 into |
hramos
removed
Import Started
Summary: This expands functionality of URL minimally so Apollo Server can run in React Native contexts. Add explicit-fail getters so undefined values won't get generated from the otherwise missing implemenation. Use of URL in apollo-server here: https://github.com/apollographql/apollo-server/blob/458bc71eadde52483ccaef209df3eb1f1bcb4424/packages/apollo-datasource-rest/src/RESTDataSource.ts#L79 Credit to my colleague dysonpro for debugging the issue and providing the initial working stub implementation. Changelog: ---------- Help reviewers and the release process by writing your own changelog entry. See http://facebook.github.io/react-native/docs/contributing#changelog for an example. [INTERNAL] [ENHANCEMENT] - Support construction, toString(), and href() of URL objects. Pull Request resolved: facebook#22901 Differential Revision: D13690954 Pulled By: cpojer fbshipit-source-id: 7966bc17be8af9bf656bffea5d530b1e626acfb3
This expands functionality of URL minimally so Apollo Server can run in React Native contexts. Add explicit-fail getters so undefined values won't get generated from the otherwise missing implemenation.
Use of URL in apollo-server here: https://github.com/apollographql/apollo-server/blob/458bc71eadde52483ccaef209df3eb1f1bcb4424/packages/apollo-datasource-rest/src/RESTDataSource.ts#L79
Credit to my colleague @dysonpro for debugging the issue and providing the initial working stub implementation.
Changelog:
Help reviewers and the release process by writing your own changelog entry. See http://facebook.github.io/react-native/docs/contributing#changelog for an example.
[INTERNAL] [ENHANCEMENT] - Support construction, toString(), and href() of URL objects.
Test Plan:
I added new unit tests for URL to cover the implementation added, based on the whatwg tests. To prevent silent or weird failures, I added explicit getters from URL public interface that throw exceptions when called. After this change, our app bundle that uses apollo-server was able to load and run.