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.

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

Type conflict when filtering detected browsers #3258

Closed
bahmutov opened this issue Jan 30, 2019 · 4 comments
Closed

Type conflict when filtering detected browsers #3258

bahmutov opened this issue Jan 30, 2019 · 4 comments
Assignees
Labels
topic: typescript type: chore Work is required w/ no deliverable to end user

Comments

@bahmutov
Copy link
Contributor

in packages/launcher/lib/detect.ts

I had to add @ts-ignore to get around TypeScript error - the uniqBy(props(['name', 'version'])) would not work because "version" property on the Browser type is optional.

screen shot 2019-01-30 at 4 07 51 pm

We probably need to be explicit and cast ourselves inside props or somehow switch type from Browser to DetectedBrowser.

See

Below is my experiments compacting and filtering

  // const removeDuplicates = uniqBy(props(['name', 'version']))
  const removeDuplicates = uniqBy((browser: Browser) => props(['name', 'version'], browser))

  // for some reason the type system does not understand that after _.compact
  // the remaining list only contains valid Browser objects
  // and we can pick "name" and "version" fields from each object
  const toUniqueBrowsers = (browsers: (Browser | boolean)[]) => {
    const justBrowsers: Browser[] = reject(isFalse)(browsers)
    return removeDuplicates(justBrowsers)
  }
@flotwig
Copy link
Contributor

flotwig commented Jan 30, 2019

image

Even with just name (a required property) there is still a warning. I'll look and see if there's any knowledge about this from Ramda.

@flotwig
Copy link
Contributor

flotwig commented Jan 30, 2019

/** returns list of detected browsers */
function detectBrowsers(goalBrowsers?: Browser[]): Bluebird<FoundBrowser[]> {
  // we can detect same browser under different aliases
  // tell them apart by the name and the version property
  if (!goalBrowsers) {
    goalBrowsers = browsers
  }
  const removeDuplicates = uniqBy((browser: FoundBrowser) => props(['name', 'version'], browser))
  return Bluebird.mapSeries(goalBrowsers, checkBrowser)
    .then(flatten)
    .then((foundBrowsers) => compact(foundBrowsers) as unknown as FoundBrowser[])
    .then(removeDuplicates)
}

This coerces all the errors away but it feels dirty casting to unknown then casting to FoundBrowser[]... though that's the workaround the warning suggests

@bahmutov
Copy link
Contributor Author

bahmutov commented Jan 30, 2019 via email

@flotwig
Copy link
Contributor

flotwig commented Feb 4, 2019

Doesn't work, it seems like the root of the issue is that Typescript thinks that compact will return a two-dimensional array, so I changed compact to this:

const compactFalse = (browsers: any[]) => compact(browsers) as FoundBrowser[]

and the warnings go away. Gonna merge this in with #3225 if there's no objection

brian-mann pushed a commit that referenced this issue Feb 13, 2019
* adding multiple possible binary names for linux

* windows launcher doesn't consider "binary", so don't pass it

* adding test for multiple binary names

* Stronger typing, clearer variable names

* Stronger typing, clearer variable names

* cleanup

* cleanup

* clean up type- why isn't this being linted?

* Add more aliases (#3217)

* launcher changes to use Browser throughout, also clarifying FoundBrowser/Browser distinction

* wip

* wip

* update tests to expect objects

* removing errant debugger calls

* Fixing tests

* desktop-gui: use displayName for display

* ' -> "

* launcher: add definitions for google chrome beta and unstable

* server: fallthrough to using chrome helper

* server: changes for run mode to pick correct version

* desktop-gui: add displayName to fixtures

* server: isolating bug with runmode

* browser was a string all along

* server: re-promisify browser detection

* launcher: remove chrome-beta for now, needs some more tweaking for that to work 100 percent

* launcher: cleaning up types

* launcher: fix type comflict when filtering browsers (#3258)

* launcher: cast Windows foundbrowsers

* launcher: mapSeries -> map

* launcher: clean up launcher, change 1 call in server to match

* launcher: test that browsers contains what we like it to

* whoops

* launcher, server: PR review changes

* server: add unit test for non-existing browser family

* server: add family to integration tests
brian-mann pushed a commit that referenced this issue Feb 16, 2019
* adding multiple possible binary names for linux

* windows launcher doesn't consider "binary", so don't pass it

* adding test for multiple binary names

* Stronger typing, clearer variable names

* Stronger typing, clearer variable names

* cleanup

* cleanup

* clean up type- why isn't this being linted?

* Add more aliases (#3217)

* launcher changes to use Browser throughout, also clarifying FoundBrowser/Browser distinction

* wip

* wip

* update tests to expect objects

* removing errant debugger calls

* Fixing tests

* desktop-gui: use displayName for display

* ' -> "

* launcher: add definitions for google chrome beta and unstable

* server: fallthrough to using chrome helper

* server: changes for run mode to pick correct version

* desktop-gui: add displayName to fixtures

* server: isolating bug with runmode

* browser was a string all along

* server: re-promisify browser detection

* launcher: remove chrome-beta for now, needs some more tweaking for that to work 100 percent

* launcher: cleaning up types

* launcher: fix type comflict when filtering browsers (#3258)

* launcher: cast Windows foundbrowsers

* launcher: mapSeries -> map

* launcher: clean up launcher, change 1 call in server to match

* launcher: test that browsers contains what we like it to

* whoops

* server: accept path in runmode

* launcher: changes for detectByPath [wip]

* server: update tests to use new errors

* launcher: error message cleanup

* launcher: detectByPath working with CLI client

* launcher: detectByPath tests

* launcher: cli client for detectByPath

* server: update error msg snapshot

* cli: allow passing --browser to open mode

* server: using --browser=/path/ works in run and open mode!!

* launcher: change displayName of custom browsers

* server: find browser with highest version property by default

* launcher: update tests, clean up types

* server: fix tests

* server: fix tests

* cli: update help snapshots

* launcher: tests

* server: wip

* server, launcher: clean up errors

* server: add unit tests for events

* server: change e2e helper to support custom browser strings in stdout

* server: e2e tests for browser by path

* server: if this break that

* server: clean up and fix? tests

* decoffeeate, entypescriptify

* server: fix test

* cli: fix whitespace

* cli: remove external browser notice

* server: detect a browser to use for the e2e launch-by-path test

* server: make stackTraceLinesRe not match all sentences with 'at' in them

* server, launcher: update 'not found at path' error msg

* server: clean up browser switch

* server: customBrowserPath

* server: update snapshots that were affected by the old stackLineRe

* server: update stubs

* server: update BROWSER_NOT_FOUND_BY_PATH to use error objects

* server: backticks in snapshots break snapshots

* server: forgetting to save without formatting will be my downfall

* server: remove comment

* desktop-gui: make custom browsers chosen

* desktop-gui, launcher: update tests
@flotwig flotwig closed this as completed Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: typescript type: chore Work is required w/ no deliverable to end user
Projects
None yet
Development

No branches or pull requests

3 participants