Add package to test public API surface used by apps #23450
Conversation
github-actions
bot
added
area: dds
There was a problem hiding this comment.
Copilot reviewed 6 out of 11 changed files in this pull request and generated no comments.
Files not reviewed (5)
- examples/utils/import-testing/biome.jsonc: Language not supported
- examples/utils/import-testing/package.json: Language not supported
- examples/utils/import-testing/src/test/tsconfig.json: Language not supported
- examples/utils/import-testing/tsconfig.json: Language not supported
- examples/utils/import-testing/.eslintrc.cjs: Evaluated as low risk
Comments suppressed due to low confidence (1)
examples/utils/import-testing/src/testExports.ts:49
- [nitpick] The ESLint rule 'unused-imports/no-unused-imports' might be a typo or misconfiguration. Consider reviewing it.
// // eslint-disable-next-line unused-imports/no-unused-imports
Co-authored-by: Jason Hartman <[email protected]>
| // Due to several of our own packages' exports failing to build with "exactOptionalPropertyTypes", | ||
| // disable it to prevent that from erroring when combined with "skipLibCheck". | ||
| "exactOptionalPropertyTypes": false, |
There was a problem hiding this comment.
Did you give things a shot without this?
I found that there were only a couple things that were impacting presence package when I was standing it up and I was able to make [compatible] changes to those types that allowed exactOptionalPropertyTypes to be true.
There was a problem hiding this comment.
Currently you get two errors in `../../../packages/dds/tree/lib/simple-tree/core/unhydratedFlexTree.d.ts . I believe it was more when I originally wrote these tests (in another package). I suspect there are other errors as well that just aren't in code depended on by the minimal stuff currently in this package.
There was a problem hiding this comment.
We should fix it: I consider these errors to be bugs found by this package. I have added a TODO to the comment to indicate this and fixed other places which had this comment.
| // Allows writing type checking expression without having to use the results. | ||
| "noUnusedLocals": false, |
There was a problem hiding this comment.
Not actually needed, right?
There was a problem hiding this comment.
Writing type tests without this is very annoying, and I don't think this enforcement is useful for this package. We do use this in the tests for this package, but allowing some type testing to be inline seems like a good thing for future users of this now that it has its own package.
There was a problem hiding this comment.
But the type tests are in the test code, right? This is for the "production" example code.
There was a problem hiding this comment.
this package is not an example or production code. The package itself is for testing things. It has both "test" and "non test" code only since we sometimes need to test importing reexports of our APIs (there have been cases where only that fails, and its been annoying for our customers). THis package simply lives in examples since there isn't a better place I'm aware of to put something which is allowed to depend on fluid-framework and other public API surfaces like our examples do.
Also writing type asserts in production code is rather useful (which is why the tree package also opts out of this compiler setting and uses the linter instead so we can have more fine grained control) so I don't think opting out of it here is odd.
| // Allow testing that declarations work properly | ||
| "declaration": true, | ||
| // Check that the Fluid Framework packages pass type checking. | ||
| "skipLibCheck": false, |
There was a problem hiding this comment.
Aren't these the common settings? ... and thus don't need to be explicit/exceptional?
There was a problem hiding this comment.
I believe we default to skipLibCheck: true
There was a problem hiding this comment.
I think that is the case for the "test" tsconfig files, but "production" tsconfigs default to false for skipLibCheck.
There was a problem hiding this comment.
Good point. You are correct (I tested it). I kept this in the config, and added an explication about it.
github-actions
bot
added
area: dev experience
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Description
The actual d.ts files we let customers use can have issues that can't be detected without importing them.
Thus to test them we need tests which can import them.
Previously some of these tests randomly lived in tree-react-utils.
This change moves those tests to a new package created for this purpose, with the expectation that other tests will be added in the future.
Reviewer Guidance
The review process is outlined on this wiki page.