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

Import explicit types from @endo/marshal #4413

Closed
wants to merge 2 commits into from

Conversation

michaelfig
Copy link
Member

@michaelfig michaelfig commented Jan 28, 2022

closes: #XXXX
refs: #XXXX

Description

Informational for @kriskowal (and @mhofman if interested). This is the other side of endojs/endo#1025 , which must be merged before this work can continue.

Security Considerations

Documentation Considerations

Testing Considerations

@michaelfig michaelfig marked this pull request as draft January 28, 2022 04:39
@michaelfig michaelfig self-assigned this Jan 28, 2022
Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Same nit that I'd prefer if we kept using file extensions in type imports.

I'm wondering if this ends up working without the maxNodeModulesJsDepth option because the types are explicitly imported?

Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

If it weren't causing lint errors, it would LGTM ;)

@mhofman mhofman changed the base branch from master to warner-update-typescript February 2, 2022 05:21
@mhofman
Copy link
Member

mhofman commented Feb 2, 2022

Rebased on top of #4420, added a few more explicit type import that were needed, and included the patch to the endo packages so that one can try this without jumping through hoops.

@mhofman
Copy link
Member

mhofman commented Feb 2, 2022

If it weren't causing lint errors

It doesn't anymore ;)

@mhofman mhofman force-pushed the warner-update-typescript branch 2 times, most recently from a9f3176 to 9dc29ca Compare February 2, 2022 23:23
@mhofman mhofman force-pushed the warner-update-typescript branch 2 times, most recently from 4449e2a to 9324cfe Compare February 3, 2022 18:33
Base automatically changed from warner-update-typescript to master February 3, 2022 19:05
@mhofman mhofman changed the title fix(store): use explicit import('@endo/marshal/exported') JSDoc Import explicit types from @endo/marshal Feb 3, 2022
@mhofman
Copy link
Member

mhofman commented Feb 4, 2022

Found a few other instances of ambient use, rebased and included the patch for the final version merged on the endo side. I think the best approach is to keep this in draft until @kriskowal bumps the versions, and then just use the first commit here.

@@ -159,7 +159,7 @@
* @property {string} 0 Kernel slot designating the device node that is the target of
* the invocation
* @property {string} 1 A string naming the method to be invoked
* @property {CapData} 2 A capdata object containing the arguments to the invocation
* @property {import('@endo/marshal').CapData<unknown>} 2 A capdata object containing the arguments to the invocation
Copy link
Member

Choose a reason for hiding this comment

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

@warner is this the right type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants