Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(doclint): correctly get versions on windows #1350

Merged
merged 1 commit into from
Mar 12, 2020
Merged

fix(doclint): correctly get versions on windows #1350

merged 1 commit into from
Mar 12, 2020

Conversation

JoelEinbinder
Copy link
Contributor

--version doesn't work on windows.

aslushnikov
aslushnikov reviewed Mar 12, 2020
View reviewed changes
Copy link
Collaborator

@aslushnikov aslushnikov left a comment

Choose a reason for hiding this comment

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

Is this the only option? winVersionInfo is uncomfortably huge)

@JoelEinbinder
Copy link
Contributor Author

I can launch and parse the user agent. Wdyt?

@aslushnikov
Copy link
Collaborator

I can launch and parse the user agent. Wdyt?

SG! Shall we do it on win-only?

@JoelEinbinder JoelEinbinder force-pushed the win_doclint branch 2 times, most recently from c84c205 to e4a0abe Compare March 12, 2020 04:34
@JoelEinbinder
Copy link
Contributor Author

I can launch and parse the user agent. Wdyt?

SG! Shall we do it on win-only?

Uploaded a version that does this. But the firefox version in the user agent is 74.0, while the one from --version is 74.0b10. I don't know if b10 means anything to anyone though.

aslushnikov
aslushnikov reviewed Mar 12, 2020
View reviewed changes
utils/doclint/cli.js Outdated

async function getVersion(browserType) {
if (os.platform() === 'win32' || os.platform() === 'cygwin') {
const browser = await browserType.launch();
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's a pity we don't have a browser.version()!

@aslushnikov
Copy link
Collaborator

Oh wait.

I just understood that firefox --version didn't work on win either =/ In this case, it's probably worth just always parse useragent across platforms - otherwise results might be inconsistent (like in firefox case).

Is it fast enough?

@JoelEinbinder
Copy link
Contributor Author

Oh wait.

I just understood that firefox --version didn't work on win either =/ In this case, it's probably worth just always parse useragent across platforms - otherwise results might be inconsistent (like in firefox case).

Is it fast enough?

It is very slow, but I can maybe condition it on release?

@JoelEinbinder
Copy link
Contributor Author

I found out that firefox supports /version on windows. This gives a big speedup, and gets us back the beta number.

@JoelEinbinder JoelEinbinder merged commit be83cba into microsoft:master Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aslushnikov aslushnikov aslushnikov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JoelEinbinder @aslushnikov