-
Notifications
You must be signed in to change notification settings - Fork 78
refactor(patterns): Deprecate sloppy
interface guard option
#2955
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
Conversation
6928018
to
ebbe21d
Compare
packages/patterns/package.json
Outdated
".": { | ||
"types": "./types.d.ts", | ||
"default": "./index.js" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still prefer to have an export * from './types.js'
in index.js
(with types.js
being a stub export {}
)
packages/patterns/types.d.ts
Outdated
@@ -0,0 +1,2 @@ | |||
export * from './src/types.js'; | |||
export * from './index.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed if switching to index.js
re-exporting types.js
import { isKey, isScalarKey } from '../src/keys/checkKey.js'; | ||
import { M } from '../src/patterns/patternMatchers.js'; | ||
import type { Key, ScalarKey } from '../src/types.js'; | ||
import type { Key, ScalarKey } from '../src/types.ts'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I prefer types tests to import from the entrypoint like a dependency would.
@mhofman I made your requested changes in 598ce27 but am now getting CI failures during
I can't seem to find any differences in the tsconfig files of other packages that use your preferred pattern, such as Also, other packages, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the confusion is where a manual .d.ts
/ .js
pair is needed, and where .ts
is sufficicient.
.d.ts
/ stub.js
only for the entrypoint. No meaningful content, just.d.ts
re-exporting.js
files, which are actually authored as.ts
. There cannot be a.ts
of the same name. We usually name this onetypes-index.{d.ts,js}
.ts
(without any.js
sibling) for type definitions. It cannot contain any runtime code. The prepack step will generate a.d.ts
file (which should be equivalent to the.ts
file), and a.js
files (which should be empty).
Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have not reviewed the actual change of the types.js -> types.ts.
e8a852b
to
3f20f4f
Compare
Refs: #1831, Agoric/agoric-sdk#11931
Description
We wish to visibly deprecate the
sloppy
option of@endo/patterns
interface guards. To avoid an explosion of@typedef
blocks, we migrate the@endo/patterns
types to TypeScript and then add the minimally required@deprecated
tags.Security Considerations
N/A
Scaling Considerations
N/A
Documentation Considerations
N/A
Testing Considerations
This modifies the implementation of the
@endo/patterns
types, but should not modify them. Since the number of lines changed is larged, we should attempt to convince ourselves that the types are in fact the same.Compatibility Considerations
N/A
Upgrade Considerations
Deprecates but does not remove the
sloppy
option.NEWS.md
is updated.