Skip to content

Conversation

@guardrex
Copy link
Collaborator

@guardrex guardrex commented Jan 11, 2022

Fixes #24521

Thanks @ylr-research! 🚀

  • Recommend that we show the procedure for opening a new tab.
  • To cover security aspects for older browsers, I include a bit on the Referrer Policy, but I need verification on the sanity of the remarks and example.
  • There are a few other NITs on the PR, and I'll be back for a broader UE pass on this topic later.

For the following remark ...

At this point, the file download is triggered, and it's generally safe to revoke the temporary object URL by calling [`revokeObjectURL`](https://developer.mozilla.org/docs/Web/API/URL/revokeObjectURL) on the URL.

I'm not keen on the "generally" part without further explanation. What does "generally" mean in this context? I think we should say when/how it might be unsafe ... or strike the "generally" language in favor of just saying that the URL should be revoked with the API as indicated. 👂

cc: @Rick-Anderson @serpent5 FYI: There's an unrelated absolute cross-link to EF Core docs in the RP tutorial that popped up on the build report. I've added a fix for it on this PR, so you can ignore it if it pops up on other PRs today ... UPDATE (1/13): We're a bit delayed here, so I moved the link fix to #24599 for immediate merge.

The preceding commands install [the Entity Framework Core tools](https://docs.microsoft.com/ef/core/get-started/overview/install#get-the-entity-framework-core-tools) and run the `migrations` command to generate code that creates the initial database schema.

... to ...

The preceding commands install [the Entity Framework Core tools](/ef/core/get-started/overview/install#get-the-entity-framework-core-tools) and run the `migrations` command to generate code that creates the initial database schema.

@guardrex guardrex requested a review from TanayParikh January 11, 2022 13:57
@dotnetspark
Copy link

I will vote for "the URL should be revoked with the API as indicated". This is perfect.

@guardrex
Copy link
Collaborator Author

Like most of the PU engineers, Tanay doesn't usually do Random™ 🤣 ... I won't be surprised if he had something specific in mind.

Copy link
Contributor

@TanayParikh TanayParikh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @guardrex for putting this together. Left some minor comments:

@TanayParikh TanayParikh self-assigned this Jan 12, 2022
@dotnetspark
Copy link

I have some sample code in this repo which received some feedback from @SteveSandersonMS. Mainly after running into some issues with memory out of bounds on large files. See 36872 and 60287

@guardrex
Copy link
Collaborator Author

guardrex commented Jan 13, 2022

Thanks @ylr-research ... I yield to @TanayParikh on those. He's a real programmer, not a HackDino™ 🦖 like me! 😄 I haven't even had time to walk through this guidance in detail and experience the code first-hand, as I've been busy 🏃🏃🏃🏃🏃😅 on a number of other article issues. I'll be returning this article later for additional updates, including ...

  • Enhanced article and API cross-linking.
  • Fully-working, cut-'n-paste example code for devs to experiment with.
  • Moving sample code into the Blazor snippet sample apps so that we catch regressions when new versions of the framework release with updated API.
  • ... and any stray COMMA or MISSPELLING 😈 that has weaseled its way into the doc!

I've cross-linked your comment to my UE ("User Experience") tracking issue so that I don't forget to check them out later when I circle back to this article.

@guardrex guardrex assigned guardrex and unassigned TanayParikh Jan 14, 2022
@guardrex guardrex merged commit 0505142 into main Jan 18, 2022
@guardrex guardrex deleted the guardrex-patch-1 branch January 18, 2022 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ArrayBuffer vs Stream

4 participants