Skip to content

[identity] Update dev dependency "open"#6278

Merged
mikeharder merged 4 commits intoAzure:masterfrom
mikeharder:update-open
Nov 21, 2019
Merged

[identity] Update dev dependency "open"#6278
mikeharder merged 4 commits intoAzure:masterfrom
mikeharder:update-open

Conversation

@mikeharder
Copy link
Member

@daviwil: Note the code change to the sample, as recommended by the docs for open@7.0.0:

Correctly handle URL and path escaping on Windows (#146) 7ef15d2
If you use open with URLs, you'll want to use the new {url: true} option.

https://github.com/sindresorhus/open/releases/tag/v7.0.0

How can I validate the change to this sample? Do any of our pipelines run the sample, or does it need to be run manually?

@daviwil
Copy link
Contributor

daviwil commented Nov 20, 2019

Unfortunately it has to be run manually because this auth flow requires the user to authenticate with AAD in the browser. I can run it with this change after I get out of the meeting I'm in.

@mikeharder
Copy link
Member Author

Thanks. Do you think we should add url: true in the sample or no? I have no preference, the doc recommends it for URLs, but I think it only matters if the URL happens to contain a double-quote character (").

@daviwil
Copy link
Contributor

daviwil commented Nov 20, 2019

I think it's a good addition here since it seems users on Windows (of which we have many) might encounter problems with launching URLs with ampersands (which we do).

@mikeharder
Copy link
Member Author

Since the URL query string in the sample is already encoded, we need to omit url: true to prevent double-encoding. An alternative fix would be to set encode: false when calling qs.stringify(), and then set url: true when calling open().

@mikeharder mikeharder merged commit c570dca into Azure:master Nov 21, 2019
@mikeharder mikeharder deleted the update-open branch November 21, 2019 00:24
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.

2 participants