Skip to content

Do not use Blob/FileReader modules in UWP - #10079

Merged
Julio César Rocha (JunielKatarn) merged 4 commits into
microsoft:mainfrom
jurocha-ms:issues/10036/uwp-noblob
Jun 9, 2022
Merged

Do not use Blob/FileReader modules in UWP#10079
Julio César Rocha (JunielKatarn) merged 4 commits into
microsoft:mainfrom
jurocha-ms:issues/10036/uwp-noblob

Conversation

@JunielKatarn

@JunielKatarn Julio César Rocha (JunielKatarn) commented Jun 7, 2022

Copy link
Copy Markdown
Contributor

Description

Do not add BlobModule nor FileReaderModule to the default CxxModule list when running a UWP MSRN instance.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

Undefined object sharing behavior in UWP (as opposed to Desktop) will cause a malfunction of HTTP/fetch workflows due to a misconfigured Blob module.

Resolves #10036

What

  • Conditionally compile the code that adds BlobModule and FileReaderModule to the default modules' list in the InstanceImpl class.
  • Defined Blob.EnableModule runtime option.
Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the Area: Networking label Jun 7, 2022
@JunielKatarn Julio César Rocha (JunielKatarn) added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 7, 2022
@ghost

Copy link
Copy Markdown

Hello Julio César Rocha (@JunielKatarn)!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 hours. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@asklar

Copy link
Copy Markdown
Member

Undefined object sharing behavior in UWP (as opposed to Desktop) will cause a malfunction of HTTP/fetch workflows due to a misconfigured Blob module.
Julio César Rocha (@JunielKatarn) could you clarify what about UWP makes this not work / how it's related to the "invalid response" error? Ideally we don't add further behavior forking unless there's a clear platform difference

@JunielKatarn

Copy link
Copy Markdown
Contributor Author

Undefined object sharing behavior in UWP (as opposed to Desktop) will cause a malfunction of HTTP/fetch workflows due to a misconfigured Blob module.
Julio César Rocha (@JunielKatarn) could you clarify what about UWP makes this not work / how it's related to the "invalid response" error? Ideally we don't add further behavior forking unless there's a clear platform difference

Updating the summary.
In short, it's the ad-hoc property bag used for the Desktop DLL which does not get used by MSRN's modules.
While we figure out a common way to initialize such property bag for both variants, the decision is not to fork functionality, but to postpone enabling Blob/FileReader in UWP.
Meaning, UWP's behavior is mean to stay unchanged for now.

@asklar

Copy link
Copy Markdown
Member

Undefined object sharing behavior in UWP (as opposed to Desktop) will cause a malfunction of HTTP/fetch workflows due to a misconfigured Blob module.
Julio César Rocha (@JunielKatarn)

        Julio Cesar Rocha
        FTE could you clarify what about UWP makes this not work / how it's related to the "invalid response" error? Ideally we don't add further behavior forking unless there's a clear platform difference

Updating the summary. In short, it's the ad-hoc property bag used for the Desktop DLL which does not get used by MSRN's modules. While we figure out a common way to initialize such property bag for both variants, the decision is not to fork functionality, but to postpone enabling Blob/FileReader in UWP. Meaning, UWP's behavior is mean to stay unchanged for now.

We have ReactInstanceSettings.Properties as the mechanism for storing/retrieving arbitrary properties in RNW
a) can Office not use that? (cc Andrew Coates (@acoates-ms) for comment)
b) Is there an issue filed/ETA to undo this workaround and get us at parity?
c) it looks like the breaking change was backported a few releases back, could you please also fix it there

I'm a little concerned that this change went through into public OSS without testing, as any basic [manual] validation with RNTester would have uncovered that this was a breaking change, and then it got backported, breaking OSS customers. I imagine this worked fine for Office Desktop but clearly not for RNW. Could we please slow down in the future and make sure we don't jam things through (esp when a change is backported) without proper validation?

@JunielKatarn

Copy link
Copy Markdown
Contributor Author

Undefined object sharing behavior in UWP (as opposed to Desktop) will cause a malfunction of HTTP/fetch workflows due to a misconfigured Blob module.
Julio César Rocha (@JunielKatarn)

        Julio Cesar Rocha
        FTE could you clarify what about UWP makes this not work / how it's related to the "invalid response" error? Ideally we don't add further behavior forking unless there's a clear platform difference

Updating the summary. In short, it's the ad-hoc property bag used for the Desktop DLL which does not get used by MSRN's modules. While we figure out a common way to initialize such property bag for both variants, the decision is not to fork functionality, but to postpone enabling Blob/FileReader in UWP. Meaning, UWP's behavior is mean to stay unchanged for now.

We have ReactInstanceSettings.Properties as the mechanism for storing/retrieving arbitrary properties in RNW a) can Office not use that? (cc Andrew Coates (@acoates-ms) for comment) b) Is there an issue filed/ETA to undo this workaround and get us at parity? c) it looks like the breaking change was backported a few releases back, could you please also fix it there

I'm a little concerned that this change went through into public OSS without testing, as any basic [manual] validation with RNTester would have uncovered that this was a breaking change, and then it got backported, breaking OSS customers. I imagine this worked fine for Office Desktop but clearly not for RNW. Could we please slow down in the future and make sure we don't jam things through (esp when a change is backported) without proper validation?

a) No. Office uses the Desktop DLL which can't access the whole React Context / ReactInstanceSettings mechanism.
b) No ETA, but it can be done as soon as we resolve the property bag-passing issue.
c) Awaiting for approval of this PR to begin backporting.

@JunielKatarn
Julio César Rocha (JunielKatarn) requested a review from a team June 8, 2022 20:43
@JunielKatarn Julio César Rocha (JunielKatarn) removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 8, 2022
@JunielKatarn
Julio César Rocha (JunielKatarn) merged commit 2983a5b into microsoft:main Jun 9, 2022
@JunielKatarn
Julio César Rocha (JunielKatarn) deleted the issues/10036/uwp-noblob branch June 9, 2022 00:34
Julio César Rocha (JunielKatarn) added a commit to jurocha-ms/react-native-windows that referenced this pull request Jun 9, 2022
* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation
Julio César Rocha (JunielKatarn) added a commit to jurocha-ms/react-native-windows that referenced this pull request Jun 9, 2022
* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation
Julio César Rocha (JunielKatarn) added a commit to jurocha-ms/react-native-windows that referenced this pull request Jun 9, 2022
* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation
Julio César Rocha (JunielKatarn) added a commit to jurocha-ms/react-native-windows that referenced this pull request Jun 9, 2022
* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation
Danny van Velzen (dannyvv) pushed a commit that referenced this pull request Jun 9, 2022
* Do not use Blob/FileReader modules in UWP (#10079)

* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation

* Change files

* clang format
Alexander Sklar (asklar) pushed a commit to asklar/react-native-windows that referenced this pull request Jun 18, 2022
* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation
ghost pushed a commit that referenced this pull request Jun 20, 2022
* Do not use Blob/FileReader modules in UWP (#10079)

* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation

* Change files

* Use logical OR to assert HTTP responseType (#10095)

* update yarn.lock

* Use logical OR to assert HTTP responseType

* Change files

* rm change file

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
ghost pushed a commit that referenced this pull request Jun 20, 2022
* Do not use Blob/FileReader modules in UWP (#10079)

* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation

* Remove change file

* Change files

* Use logical OR to assert HTTP responseType (#10095)

* update yarn.lock

* Use logical OR to assert HTTP responseType

* Change files

* rm change file

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
ghost pushed a commit that referenced this pull request Jun 20, 2022
* Do not use Blob/FileReader modules in UWP (#10079)

* Do not use Blob/FileReader modules on UWP

* Change files

* Upgrade packages.lock.json

* Use runtime option Blob.EnableModule to condition instantiation

* Change files

* Use logical OR to assert HTTP responseType (#10095)

* update yarn.lock

* Use logical OR to assert HTTP responseType

* Change files

* rm change file

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
@omniboyOK

Copy link
Copy Markdown

I'm having this same issue after installing 0.70 today.

@JunielKatarn

Copy link
Copy Markdown
Contributor Author

I'm having this same issue after installing 0.70 today.

Can you please elaborate?
After this PR was merged, we managed to enable Blob support.

What error are you getting?
A GitHub issue with specific repro steps would be much appreciated :)

@jbcullis

Copy link
Copy Markdown

Same issue here - error is "Invalid response for blob"

@tero-paananen

Copy link
Copy Markdown

I have this error now on 0.69.11. Issue did not exist on 0.69.7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid response for blob

6 participants