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

build(merge-tree,sequence): Fix type generation for ESM and add exports field #18825

Merged
merged 6 commits into from Dec 14, 2023

Conversation

tylerbutler
Copy link
Member

@tylerbutler tylerbutler commented Dec 14, 2023

This PR updates sequence and merge-tree to generate proper types in ESM builds and adds an exports field. I had to update the test files a bit to clean up imports. I also added a ./dist/test export from merge-tree since its internals are used by sequence tests. This is a demonstration of the mechanism we will have to use any time we want to export something for use in another package. No more reaching into internals!

This PR includes some configuration and settings changes that are not strictly needed for this PR, but splitting them out doesn't seem worth the time since they're all needed for other pending PRs like #18823 and #18824.

@github-actions github-actions bot added area: build Build related issues area: dds Issues related to distributed data structures area: dds: sharedstring dependencies Pull requests that update a dependency file base: main PRs targeted against main branch labels Dec 14, 2023
@github-actions github-actions bot added the area: runtime Runtime related issues label Dec 14, 2023
@msfluid-bot
Copy link
Collaborator

msfluid-bot commented Dec 14, 2023

@fluid-example/bundle-size-tests: -233.95 KB
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 597.41 KB 503.6 KB -93.81 KB
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 293.21 KB 291.51 KB -1.71 KB
loader.js 188.15 KB 187.22 KB -953 Bytes
map.js 99.22 KB 99.02 KB -201 Bytes
matrix.js 242.13 KB 198.33 KB -43.8 KB
odspDriver.js 112 KB 111.62 KB -398 Bytes
odspPrefetchSnapshot.js 67.4 KB 67.25 KB -154 Bytes
sharedString.js 306.95 KB 216.16 KB -90.8 KB
sharedTree2.js 323.65 KB 321.63 KB -2.02 KB
Total Size 2.29 MB 2.06 MB -233.95 KB

Baseline commit: eb21fdd

Generated by 🚫 dangerJS against f9bbe73

Comment on lines +25 to +34
"./dist/test": {
"import": {
"types": "./dist/test/index.d.ts",
"default": "./dist/test/index.cjs"
},
"require": {
"types": "./dist/test/index.d.ts",
"default": "./dist/test/index.cjs"
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
"./dist/test": {
"import": {
"types": "./dist/test/index.d.ts",
"default": "./dist/test/index.cjs"
},
"require": {
"types": "./dist/test/index.d.ts",
"default": "./dist/test/index.cjs"
}
}
"./dist/test": {
"types": "./dist/test/index.d.ts",
"default": "./dist/test/index.cjs"
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Not going to risk changing it, though.

@tylerbutler tylerbutler marked this pull request as ready for review December 14, 2023 18:05
@tylerbutler tylerbutler requested review from msfluid-bot and a team as code owners December 14, 2023 18:05
@tylerbutler tylerbutler changed the title Update merge-tree and sequence to build correctly with moduleResolution: node16 build(merge-tree,sequence): Fix type generation for ESM and add exports field Dec 14, 2023
fluidBuild.config.cjs Outdated Show resolved Hide resolved
"default": "./dist/index.cjs"
}
},
"./dist/test": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to just call this ./test? Would clean up some import patterns nicely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but in a future change, because doing that require more churn.

fluidBuild.config.cjs Outdated Show resolved Hide resolved
@tylerbutler tylerbutler merged commit de43f3c into microsoft:main Dec 14, 2023
35 checks passed
@tylerbutler tylerbutler deleted the node16-part-2 branch December 14, 2023 20:36
tyler-cai-microsoft pushed a commit to tyler-cai-microsoft/FluidFramework that referenced this pull request Jan 10, 2024
…ts field (microsoft#18825)

This PR updates sequence and merge-tree to generate proper types in ESM
builds and adds an exports field. I had to update the test files a bit
to clean up imports. I also added a ./dist/test export from merge-tree
since its internals are used by sequence tests. This is a demonstration
of the mechanism we will have to use any time we want to export
something for use in another package. No more reaching into internals!

This PR includes some configuration and settings changes that are not
strictly needed for this PR, but splitting them out doesn't seem worth
the time since they're all needed for other pending PRs like microsoft#18823 and
microsoft#18824.
tyler-cai-microsoft pushed a commit to tyler-cai-microsoft/FluidFramework that referenced this pull request Jan 10, 2024
…ts field (microsoft#18825)

This PR updates sequence and merge-tree to generate proper types in ESM
builds and adds an exports field. I had to update the test files a bit
to clean up imports. I also added a ./dist/test export from merge-tree
since its internals are used by sequence tests. This is a demonstration
of the mechanism we will have to use any time we want to export
something for use in another package. No more reaching into internals!

This PR includes some configuration and settings changes that are not
strictly needed for this PR, but splitting them out doesn't seem worth
the time since they're all needed for other pending PRs like microsoft#18823 and
tyler-cai-microsoft added a commit that referenced this pull request Jan 10, 2024
Ported PRs: 
- #18822
- #18825

## Description
Our partner team notified us of a 150kb increase in size when consuming
`2.0.0-internal.8.0.0`. After an investigation into our bundle size
telemetry, it was determined that these two commits in main had reduced
the bundle size by about ~100kb.

## Impact
Reduce bundle size by ~100kb.

---------

Co-authored-by: Tyler Butler <tylerbu@microsoft.com>
tyler-cai-microsoft added a commit that referenced this pull request Jan 10, 2024
Ported PRs: 
- #18822
- #18825

## Description
Our partner team notified us of a 150kb increase in size when consuming
`2.0.0-internal.7.4.x`. After an investigation into our bundle size
telemetry, it was determined that these two commits in main had reduced
the bundle size by about ~100kb.

## Impact
Reduce bundle size by ~100kb.

---------

Co-authored-by: Tyler Butler <tylerbu@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: build Build related issues area: dds: sharedstring area: dds Issues related to distributed data structures area: runtime Runtime related issues base: main PRs targeted against main branch dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants