fix: Add types to package export map #569
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@StoneCypher Did you have a chance to think about this change? We've been successfully using a patched version of jssm with this change for some time without issues, though to be fair we use jssm in our build tools, not product code, so take this anecdotal data with a grain of salt. That said, I think this is a safe change and doesn't represent any runtime changes - all the changes are in the type layer. |
|
Have you looked at the new package stuff in typescript from yesterday yet? I'm inclined to land this but I haven't figured out testing yet |
tylerbutler
added a commit
to microsoft/FluidFramework
that referenced
this pull request
Oct 25, 2024
The latest release of jssm includes the fixes in StoneCypher/jssm#569, so our patch is no longer needed and has been removed. However, the jssm-viz package has a similar problem (open PR: StoneCypher/jssm-viz#54) so I patched it with type changes similar to what was done for jssm originally. If or when the jssm-viz PR is accepted or the issue is otherwise fixed, we can remove the patch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a possible fix for StoneCypher/fsl#1295.
As noted in that issue, TypeScript is unable to find the included jssm types in some configurations. Part of the problem is that there are no types in the export map, so this PR updates the export map to include
typesentries.I also created a rolled-up declaration for ESM and CJS using rollup-plugin-dts. This seems to work well in my testing and is the smallest fix I know of. I prototyped those changes and tested them on the repro project at https://github.com/tylerbutler/jssm-node16 and compilation worked as expected for both CJS and ESM.
Because jssm is using Rollup v2, I used an older release of rollup-plugin-dts since the most recent version requires Rollup 3.
Possible risks:
As it stands today without this change, TypeScript projects using more recent
moduleResolutionsettings will fail to compile outright. With this change, those projects can compile and will see jssm types. However, there is a risk that rollup-plugin-dts produces invalid or flawed declaration files, or that there are further types-related issues that are uncovered by this change.That said, this change should be exceedingly safe in terms of runtime behavior, because all the changes are limited to the types-level only, and thus only affect compilation in TypeScript projects. The runtime jssm JS has not changed.