Fix withCredentials flag for fetch requests
#14931
Closed
Conversation
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
|
Can this be considered for 0.47? @grabbou This fixes a bug introduced in 0.46. |
|
cc @rigdern |
|
I'm not convinced this is the right fix. My doubts stem from whether #10575 (which introduced the regression) is correct behavior in the first place. We're kinda caught in a hard spot here, because according to the poll and heated debate in #14063 (comment), a majority of people prefer to respect the underlying platform behaviors in regards to cookies/credentials. However, in #10575, code was merged to try and emulate behavior from the web. At the very least, I think we need to be consistent: either let the individual native platform defaults prevail, or coerce each platform into emulating the behavior of the web. If we choose the former, we should wholesale revert #10575. People who are interested in a cookie jar that is shared between vanilla HTTP and WebSockets will need to roll a custom solution (or explicitly provide a If we choose the latter, we need to ensure that the anticipated behavior for both values of I hate to reprise a religious debate, but I'd like to make sure that we're at least consistent in our behavior, so as to mitigate (since we cannot completely eliminate) surprise to the developer. cc'ing people from the earlier cookie behavior debate: @ericvicenti @rigdern @talkol @satya164 |
|
Given the relative lack of interest, I'm going to propose a PR to revert #10575. Maybe that will spark up the conversation we need to have about these breakages. |
|
I agree with @jamesreggio actually, my PR is not the right way to do it. |
|
@JonnyBurger, I opened a PR to revert the original breaking change in #16127. If you agree, do you mind chiming in? |
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 27, 2019
Summary: This sync includes the following changes: - **[8e25ed20b](facebook/react@8e25ed20b )**: Unify noop and test renderer assertion APIs (#14952) //<Andrew Clark>// - **[870214f37](facebook/react@870214f37 )**: Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps (#14912) //<Eli White>// - **[3989c0950](facebook/react@3989c0950 )**: eslint-plugin-react-hooks@1.3.0 //<Dan Abramov>// - **[1bbfbc98d](facebook/react@1bbfbc98d )**: [ESLint] Add more cases to exhaustive-deps rule (#14930) //<Dan Abramov>// - **[412f88296](facebook/react@412f88296 )**: fix(eslint-plugin-react-hooks): node engine updated to version 7 because of object.entries(#14951) //<Farhad Yasir>// - **[ba708fa79](facebook/react@ba708fa79 )**: Remove ReactNoop.flushDeferredPri and flushUnitsOfWork (#14934) //<Andrew Clark>// - **[920b0bbb3](facebook/react@920b0bbb3 )**: [scheduler] Pass didTimeout argument to callbacks (#14931) //<Andrew Clark>// - **[f99fca3cb](facebook/react@f99fca3cb )**: Fix sample ESLint configuration (#14926) //<Matt Thomson>// - **[22bb94764](facebook/react@22bb94764 )**: Release eslint-plugin-react-hooks@1.2.0 //<Dan Abramov>// - **[a77bbf1a1](facebook/react@a77bbf1a1 )**: [ESLint] Warn against assignments from inside Hooks (#14916) //<Dan Abramov>// - **[219ce8a9c](facebook/react@219ce8a9c )**: Fix tracing fixture (#14917) //<Dan Abramov>// - **[8c1966590](facebook/react@8c1966590 )**: Release 16.8.3 //<Dan Abramov>// - **[7de4d2391](facebook/react@7de4d2391 )**: Fix UMD builds by re-exporting the scheduler priorities (#14914) //<Dan Abramov>// - **[d0318fb3f](facebook/react@d0318fb3f )**: Updating copyright headers, dropping the year (#14893) //<Nathan Hunzaker>// - **[f978d5fde](facebook/react@f978d5fde )**: Fix warning message for new setNativeProps method. on -> with (#14909) //<Eli White>// - **[b0f45c0fc](facebook/react@b0f45c0fc )**: Adding ReactNative.setNativeProps that takes a ref (#14907) //<Eli White>// - **[4f4aa69f1](facebook/react@4f4aa69f1 )**: Adding setNativeProps tests for NativeMethodsMixin (#14901) //<Eli White>// - **[b96b61dc4](facebook/react@b96b61dc4 )**: Use the canonical nativeTag for Fabric's setNativeProps (#14900) //<Eli White>// - **[dab2fdbbb](facebook/react@dab2fdbbb )**: Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies (#14636) //<Dan Abramov>// - **[1493abd7e](facebook/react@1493abd7e )**: Deleted empty App.css (#14149) //<Josh R>// - **[13645d224](facebook/react@13645d224 )**: Deal with fallback content in Partial Hydration (#14884) //<Sebastian Markbåge>// - **[c506ded3b](facebook/react@c506ded3b )**: Don't discard render phase state updates with the eager reducer optimization (#14852) //<Dan Abramov>// - **[0e67969cb](facebook/react@0e67969cb )**: Prompt to include UMD build artifact links in GitHub release (#14864) //<Brian Vaughn>// - **[fad0842fd](facebook/react@fad0842fd )**: Release scripts documentation (#14863) //<Brian Vaughn>// - **[ab7a67b1d](facebook/react@ab7a67b1d )**: Fix react-dom/server context leaks when render stream destroyed early (#14706) //<overlookmotel>// - **[3e5556043](facebook/react@3e5556043 )**: Release 16.8.2 //<Dan Abramov>// - **[dfabb77a9](facebook/react@dfabb77a9 )**: Include another change in 16.8.2 //<Dan Abramov>// - **[c555c008b](facebook/react@c555c008b )**: Include component stack in 'act(...)' warning (#14855) //<Sunil Pai>// - **[ff188d666](facebook/react@ff188d666 )**: Add React 16.8.2 changelog (#14851) //<Dan Abramov>// - **[c4d8ef643](facebook/react@c4d8ef643 )**: Fix typo in code comment (#14836) //<Deniz Susman>// - **[08e955435](facebook/react@08e955435 )**: Statically enable suspense/partial hydration flag in www (#14842) //<Sebastian Markbåge>// - **[0e4135e8c](facebook/react@0e4135e8c )**: Revert "[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802)" (#14839) //<Dan Abramov>// - **[6d4038f0a](facebook/react@6d4038f0a )**: [ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802) //<Rodrigo Ribeiro>// - **[fa6205d52](facebook/react@fa6205d52 )**: Special case crossOrigin for SVG image elements (#14832) //<Brandon Dail>// - **[c6bee765b](facebook/react@c6bee765b )**: Remove false positive warning and add TODOs about `current` being non-null (#14821) //<DanAbramov>// - **[3ae94e188](facebook/react@3ae94e188 )**: Fix ignored sync work in passive effects (#14799) //<Dan Abramov>// - **[f3a14951a](facebook/react@f3a14951a )**: Partial Hydration (#14717) //<Sebastian Markbåge>// Changelog: [GENERAL] [Changed] React sync for revisions f24a0da...22bb947 Reviewed By: gaearon Differential Revision: D14160361 fbshipit-source-id: fffdc922f3ee5dfeeee656a8f213a6d3c03e8481
grabbou
pushed a commit
that referenced
this pull request
Feb 27, 2019
Summary: This sync includes the following changes: - **[8e25ed20b](facebook/react@8e25ed20b )**: Unify noop and test renderer assertion APIs (#14952) //<Andrew Clark>// - **[870214f37](facebook/react@870214f37 )**: Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps (#14912) //<Eli White>// - **[3989c0950](facebook/react@3989c0950 )**: eslint-plugin-react-hooks@1.3.0 //<Dan Abramov>// - **[1bbfbc98d](facebook/react@1bbfbc98d )**: [ESLint] Add more cases to exhaustive-deps rule (#14930) //<Dan Abramov>// - **[412f88296](facebook/react@412f88296 )**: fix(eslint-plugin-react-hooks): node engine updated to version 7 because of object.entries(#14951) //<Farhad Yasir>// - **[ba708fa79](facebook/react@ba708fa79 )**: Remove ReactNoop.flushDeferredPri and flushUnitsOfWork (#14934) //<Andrew Clark>// - **[920b0bbb3](facebook/react@920b0bbb3 )**: [scheduler] Pass didTimeout argument to callbacks (#14931) //<Andrew Clark>// - **[f99fca3cb](facebook/react@f99fca3cb )**: Fix sample ESLint configuration (#14926) //<Matt Thomson>// - **[22bb94764](facebook/react@22bb94764 )**: Release eslint-plugin-react-hooks@1.2.0 //<Dan Abramov>// - **[a77bbf1a1](facebook/react@a77bbf1a1 )**: [ESLint] Warn against assignments from inside Hooks (#14916) //<Dan Abramov>// - **[219ce8a9c](facebook/react@219ce8a9c )**: Fix tracing fixture (#14917) //<Dan Abramov>// - **[8c1966590](facebook/react@8c1966590 )**: Release 16.8.3 //<Dan Abramov>// - **[7de4d2391](facebook/react@7de4d2391 )**: Fix UMD builds by re-exporting the scheduler priorities (#14914) //<Dan Abramov>// - **[d0318fb3f](facebook/react@d0318fb3f )**: Updating copyright headers, dropping the year (#14893) //<Nathan Hunzaker>// - **[f978d5fde](facebook/react@f978d5fde )**: Fix warning message for new setNativeProps method. on -> with (#14909) //<Eli White>// - **[b0f45c0fc](facebook/react@b0f45c0fc )**: Adding ReactNative.setNativeProps that takes a ref (#14907) //<Eli White>// - **[4f4aa69f1](facebook/react@4f4aa69f1 )**: Adding setNativeProps tests for NativeMethodsMixin (#14901) //<Eli White>// - **[b96b61dc4](facebook/react@b96b61dc4 )**: Use the canonical nativeTag for Fabric's setNativeProps (#14900) //<Eli White>// - **[dab2fdbbb](facebook/react@dab2fdbbb )**: Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies (#14636) //<Dan Abramov>// - **[1493abd7e](facebook/react@1493abd7e )**: Deleted empty App.css (#14149) //<Josh R>// - **[13645d224](facebook/react@13645d224 )**: Deal with fallback content in Partial Hydration (#14884) //<Sebastian Markbåge>// - **[c506ded3b](facebook/react@c506ded3b )**: Don't discard render phase state updates with the eager reducer optimization (#14852) //<Dan Abramov>// - **[0e67969cb](facebook/react@0e67969cb )**: Prompt to include UMD build artifact links in GitHub release (#14864) //<Brian Vaughn>// - **[fad0842fd](facebook/react@fad0842fd )**: Release scripts documentation (#14863) //<Brian Vaughn>// - **[ab7a67b1d](facebook/react@ab7a67b1d )**: Fix react-dom/server context leaks when render stream destroyed early (#14706) //<overlookmotel>// - **[3e5556043](facebook/react@3e5556043 )**: Release 16.8.2 //<Dan Abramov>// - **[dfabb77a9](facebook/react@dfabb77a9 )**: Include another change in 16.8.2 //<Dan Abramov>// - **[c555c008b](facebook/react@c555c008b )**: Include component stack in 'act(...)' warning (#14855) //<Sunil Pai>// - **[ff188d666](facebook/react@ff188d666 )**: Add React 16.8.2 changelog (#14851) //<Dan Abramov>// - **[c4d8ef643](facebook/react@c4d8ef643 )**: Fix typo in code comment (#14836) //<Deniz Susman>// - **[08e955435](facebook/react@08e955435 )**: Statically enable suspense/partial hydration flag in www (#14842) //<Sebastian Markbåge>// - **[0e4135e8c](facebook/react@0e4135e8c )**: Revert "[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802)" (#14839) //<Dan Abramov>// - **[6d4038f0a](facebook/react@6d4038f0a )**: [ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802) //<Rodrigo Ribeiro>// - **[fa6205d52](facebook/react@fa6205d52 )**: Special case crossOrigin for SVG image elements (#14832) //<Brandon Dail>// - **[c6bee765b](facebook/react@c6bee765b )**: Remove false positive warning and add TODOs about `current` being non-null (#14821) //<DanAbramov>// - **[3ae94e188](facebook/react@3ae94e188 )**: Fix ignored sync work in passive effects (#14799) //<Dan Abramov>// - **[f3a14951a](facebook/react@f3a14951a )**: Partial Hydration (#14717) //<Sebastian Markbåge>// Changelog: [GENERAL] [Changed] React sync for revisions f24a0da...22bb947 Reviewed By: gaearon Differential Revision: D14160361 fbshipit-source-id: fffdc922f3ee5dfeeee656a8f213a6d3c03e8481
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR changes
RCTNetworking.mmso that the cookies are managed usingHTTPShouldHandleCookiesif thewithCredentialsflag is set to true.This was the behavior in react-native@0.45, but was broken in react-native@0.46 where the cookies would be overwritten no matter what the
withCredentialsflag is set to.This PR restores the behavior of the
withCredentialsflag from react-native@0.45, but still uses the new cookie management from react-native@0.46 if that flag is not set.I have documented the issue and explained why this code change is necessary here: #14869