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

Use a type-safe way is differentiating ExecutionResult from Array<Error> #3649

Closed
wants to merge 2 commits into from

Conversation

matthargett
Copy link

Motivation

Checking a string key on an array is not valid, correctly yielding a warning on a TS branch I'm playing in.
Narrow the type with Array.isArray instead.

Performance

Running yarn benchmark (which is amazing, btw) on my 2019 MacBook Pro running macOS 12.4 and node 18.2.0, I see a few slight increases and slight decreases of my local vs HEAD. I exited all other user processes on the system, but there was still some slight variability from run to run regardless.

Here's a screenshot of the run, let me know if I need to dive into CPU profiler data to find another type-safe alternative that's cheaper at runtime.
image

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 17, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: matthargett / name: Matt Hargett (b76e38c)

@netlify
Copy link

netlify bot commented Jun 17, 2022

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit f488b12
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62ac0db1352f460008f7597e
😎 Deploy Preview https://deploy-preview-3649--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@yaacovCR
Copy link
Contributor

Checks failing because lockfile does not match, our ci requires you to update this yourself and then we check to make sure it matches.

Putting together your typescript version changes and your proposed code change, it sounds like you are saying that on newest version of typescript, we get a warning on our explicit property check?

It could be the simplest thing is to just disable the warning for that line.

@yaacovCR
Copy link
Contributor

Could be also that this is not secondary to the version change and just a different tsconfig in your project? I skimmed the TS 4.7 release notes and didn't note anything at first glance.

@saihaj

This comment has been minimized.

@github-actions
Copy link

@github-actions run-benchmark

@saihaj Something went wrong, please check log.

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this pull request Jul 7, 2022
As part of graphql#3649 research I added all options explicitly and also
enable ones that didn't require any code change.
We should enable rest of them in a separate PRs.
IvanGoncharov added a commit that referenced this pull request Jul 7, 2022
As part of #3649 research I added all options explicitly and also
enable ones that didn't require any code change.
We should enable rest of them in a separate PRs.
@matthargett
Copy link
Author

Superseded by #3670

@matthargett matthargett deleted the explicit-array-check branch September 19, 2023 22:58
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.

3 participants