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

fix: Correct comparison for typeof check. #308

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

03eltond
Copy link
Contributor

@03eltond 03eltond commented Nov 1, 2023

Requirements

  • I have added test coverage for new or changed functionality
    *** I cannot validate tests execute due to following error during npm install:
    npm ERR! code E404
    npm ERR! 404 Not Found - GET https://registry.npmjs.org/@launchdarkly%2fprivate-js-mocks - Not found
    npm ERR! 404
    npm ERR! 404 '@launchdarkly/[email protected]' is not in the npm registry.
    npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
    npm ERR! 404 It was specified as a dependency of 'sdk-server'
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Describe the solution you've provided

This fixes the following error on Node.js v14:

ReferenceError: performance is not defined
at Migration.trackLatency (//node_modules/@launchdarkly/js-server-sdk-common/dist/Migration.js:245:13)

Since "undefined" !== undefined 😛

Additional context

The performance module is available in Node 14, however, it is not available globally until Node 16. An ideal fix may be to include an import of performance from perf_hooks - see https://stackoverflow.com/questions/46436943/referenceerror-performance-is-not-defined-when-using-performance-now

I originally wanted to add this in, but would be unable to validate that change due to npm install errors around a private package described above. Regardless, this fix appears to work using the existing fallback mechanism using Date objects.

@kinyoklion kinyoklion changed the title fix typeof check fix: Correct comparison for typeof check. Nov 1, 2023
@kinyoklion
Copy link
Member

Thank you @03eltond

This change generally looks good, updated your title for conventional commits and made a suggestion for our prettier config.

This is intended, beyond node, to allow these checks in various edge environments as well (vercel, cloudflare, akamai). Which may have varying levels of support, so perf_hooks would be similarly problematic.

Thank you for finding this.

Thanks,
Ryan

@kinyoklion kinyoklion merged commit 568f2ab into launchdarkly:main Nov 1, 2023
15 checks passed
@github-actions github-actions bot mentioned this pull request Nov 1, 2023
@kinyoklion
Copy link
Member

@03eltond @launchdarkly/[email protected] has been released this this change.

Thanks,
Ryan

@03eltond
Copy link
Contributor Author

03eltond commented Nov 1, 2023

Thank you for the super fast turnaround!

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