Skip to content

Release/3.0.0 #492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 184 commits into from
Aug 9, 2021
Merged

Release/3.0.0 #492

merged 184 commits into from
Aug 9, 2021

Conversation

nikithauc
Copy link
Contributor

Closed #480 and create a new PR containing the support for MSAL Browser.

This release contains:
Issues in MileStone 3.0.0

V3 Upgrade Guide

nokafor and others added 30 commits December 29, 2019 13:37
dependabot bot and others added 18 commits May 31, 2021 14:59
Bumps [ws](https://github.com/websockets/ws) from 7.1.2 to 7.4.6.
- [Release notes](https://github.com/websockets/ws/releases)
- [Commits](websockets/ws@7.1.2...7.4.6)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Nikitha Chettiar <[email protected]>
* Adding custom hosts options

* Adding unit tests

* adding custom host check in telemetry handler

* Update to comments

* Function to check valid custom host

* unit test for invalid hostname
* Fixing links

* Update v3-upgrade-guide.md
* Adding the ChunkRecord interface

* Adding more tests for stream resume

* comments, assert conditions change

* SliceRecord and comments
* 3.0.0 preview 2 package update

* restore file
* adding links to samples

* removing duplicate links

* adding description for typings and removing en-us
* removing node and browser; adding largefile samples

* adding links of samples in docs

* token credential readme change
* creating msal browser auth provider

* unit test, msal browser options

* error handling unit test, doc updates

* rename auth provider

* doc and folder name update

* throw error, msal cdn reference
* 3.0.0-preview-3 release

* msal updates
@nikithauc nikithauc requested review from zengin, baywet and MIchaelMainer and removed request for zengin August 2, 2021 18:52
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

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

@nikithauc I'm curious to why you closed and re-opened a PR for the same thing?
This practice has multiple negative impacts:

  • We had 12 comments that we still unresovled in the previous PR that we now need to track manually.
  • It also looses the "viewed" status on files, pushing additional burden on reviewers to view again files they have already reviewed. Which is especially painful on large pull requests. (see screenshot below)

I'm not going to review 180 files entirely a second time.

Thanks for taking in some of the initial comments, especially around the dependencies and auth topics.

image

@nikithauc
Copy link
Contributor Author

@baywet

The merge was messy because of which I decided to recreate this release branch. I wanted to avoid the risk of wrong conflict resolution and keep this release branch close to the latest preview release.

The new commits introduced in this PR after #480 are:

The unresolved comment in #480 have been mapped to issues and will be tracked as enhancement issues.

@nikithauc nikithauc merged commit a3ee95d into master Aug 9, 2021
@baywet
Copy link
Member

baywet commented Aug 10, 2021

@nikithauc Thanks for the additional context.

Can I suggest the following alternatives whenever you want to "tidy-up" a pull request:

  • hide comments by marking them as resolved/outdated: this works both for review comments as you already know and also for discussions comments (see the 3 dots in the top right corner of a comment). I wouldn't advice deleting them however, as this would lose valuable context.
  • rebasing & force-push
  • new local branch, cherry-pick, rebase the original branch off this local, force push

Again, re-opening pull requests adds burden not only for the reviewers, but also for anybody who's following the repo (additional notifications which force people to triage things another time).

@baywet baywet deleted the release/3.0.0 branch September 19, 2023 18:19
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.

9 participants