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(client-utils): fix trace.js import of performance #21257

Merged
merged 10 commits into from
Jun 4, 2024
Merged

fix(client-utils): fix trace.js import of performance #21257

merged 10 commits into from
Jun 4, 2024

Conversation

DLehenbauer
Copy link
Contributor

Fix GH #21252

@github-actions github-actions bot added the base: main PRs targeted against main branch label May 30, 2024
@@ -3,7 +3,7 @@
* Licensed under the MIT License.
*/

import { performance } from "./indexNode.js";
import { performance } from "./performanceIsomorphic.js";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the actual fix. There is no node/browser specific code involved. PerformanceIsomorphic just narrows the type of the global performance object to the intersection of what browser/node support.

@DLehenbauer DLehenbauer marked this pull request as ready for review May 31, 2024 20:06
@DLehenbauer DLehenbauer requested a review from jason-ha May 31, 2024 20:20
Copy link
Contributor

@jason-ha jason-ha left a comment

Choose a reason for hiding this comment

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

Looks like the buffer tests in jest have some typos. There are some things that refer to Node or Browser but are actually the other. The first I noticed was in "Views are supported".

Some small buffer test in mocha would be nice.

Ideally, we'd have tests that used the public interface - @fluid-internal/client-utils. That is entirely possible with either:

  1. moving tests out from under dist | lib output
  2. cloning the exports package.json (easier now that they are smaller; restore the file deleted) See also AB#7374.
  3. not testing CJS (per the package.json not having appropriate exports)

packages/common/client-utils/README.md Outdated Show resolved Hide resolved
packages/common/client-utils/package.json Show resolved Hide resolved
packages/common/client-utils/src/base64EncodingBrowser.ts Outdated Show resolved Hide resolved
packages/common/client-utils/src/test/mocha/trace.spec.ts Outdated Show resolved Hide resolved
Co-authored-by: Jason Hartman <jasonha@microsoft.com>
@DLehenbauer DLehenbauer enabled auto-merge (squash) June 4, 2024 21:31
@DLehenbauer DLehenbauer merged commit 925a477 into main Jun 4, 2024
30 checks passed
@DLehenbauer DLehenbauer deleted the trace branch June 4, 2024 22:16
scottn12 added a commit to scottn12/FluidFramework that referenced this pull request Jun 5, 2024
Fix GH microsoft#21252

---------

Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 pushed a commit to scottn12/FluidFramework that referenced this pull request Jun 5, 2024
Fix GH microsoft#21252

---------

Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 pushed a commit to scottn12/FluidFramework that referenced this pull request Jun 5, 2024
Fix GH microsoft#21252

---------

Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 pushed a commit to scottn12/FluidFramework that referenced this pull request Jun 5, 2024
Fix GH microsoft#21252

---------

Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 pushed a commit to scottn12/FluidFramework that referenced this pull request Jun 5, 2024
Fix GH microsoft#21252

---------

Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 added a commit that referenced this pull request Jun 5, 2024
Fix GH #21252

Cherry pick of
925a477

Co-authored-by: Daniel Lehenbauer <DLehenbauer@users.noreply.github.com>
Co-authored-by: Jason Hartman <jasonha@microsoft.com>
scottn12 added a commit that referenced this pull request Jun 5, 2024
Fix GH #21252

Cherry pick of
925a477

Co-authored-by: Daniel Lehenbauer <DLehenbauer@users.noreply.github.com>
Co-authored-by: Jason Hartman <jasonha@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants