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.

Sign up for GitHub

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

Jump to bottom

no more maxNodeModuleJsDepth #6369

Merged
merged 4 commits into from
Oct 25, 2022
Merged

no more maxNodeModuleJsDepth #6369

merged 4 commits into from
Oct 25, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Sep 30, 2022

Description

Progress on #4560

Depends on endojs/endo#1307

I made the Endo changes and then used yarn link to test them in this repo. I stopped when I ran into bundle-source and ses-ava problems, which I've added to the priority list in endojs/endo#1254

Security Considerations

--

Documentation Considerations

--

Testing Considerations

--

@mhofman
Copy link
Member

mhofman commented Sep 30, 2022 •
edited
Loading

I made the Endo changes and then used yarn link to test them in this repo.

Do we know for sure that symlinks work the same as regular node_modules folders for tsc? I'd feel more confident if we did yarn pack and maybe a temporary package.json resolutions to use the updated endo packages

kriskowal
kriskowal approved these changes Oct 1, 2022
View reviewed changes
Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

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

Squee.

@turadg
Copy link
Member Author

turadg commented Oct 1, 2022

Do we know for sure that symlinks work the same as regular node_modules folders for tsc?

Not yet. This won't pass until the changes in endojs/endo#1307 are available in NPM. Looks like rn that will take a release crank. @kriskowal could we get dev or nightly releases for Endo to reduce the friction in this cross-repo changes?

@turadg turadg force-pushed the ta/fewer-maxNodeModuleJsDepth branch from fcc6573 to 7c96d99 Compare October 24, 2022 23:12
@turadg turadg changed the base branch from master to turadg-sync-endo-2022-10-24-21-57-11 October 24, 2022 23:17
@turadg
Copy link
Member Author

turadg commented Oct 24, 2022

I think once #6493 lands I can get this to complete #4560 (by hook and a little crook)

Base automatically changed from turadg-sync-endo-2022-10-24-21-57-11 to master October 25, 2022 00:58
@turadg turadg force-pushed the ta/fewer-maxNodeModuleJsDepth branch from 7c96d99 to 17e91dc Compare October 25, 2022 17:30
@turadg turadg requested a review from mhofman October 25, 2022 17:30
mhofman
mhofman requested changes Oct 25, 2022
View reviewed changes
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.

Blocker is the import of d.ts file. I also would like to understand why that didn't cause any test to fail.

packages/zoe/src/contractSupport/priceAuthority.js Outdated Show resolved Hide resolved
packages/internal/src/config.js Outdated Show resolved Hide resolved
packages/notifier/src/asyncIterableAdaptor.js
@@ -80,6 +80,8 @@ export const makeAsyncIterableFromNotifier = notifierP => {
return harden({ value, done });
});
}
// xxx hint for type checker
assert(myIterationResultP);
Copy link
Member

Choose a reason for hiding this comment

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

Ugh that's weird. Why doesn't flow control realize that myIterationResultP is never undefined here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's weird. I don't now why but here's the error without it,

> @agoric/[email protected] lint:types
> tsc -p jsconfig.json

../notifier/src/asyncIterableAdaptor.js(39,3): error TS2322: Type '{ [Symbol.asyncIterator]: () => { next: () => Promise<{ value: T; done: boolean; }> | undefined; } & RemotableBrand<{}, { next: () => Promise<{ value: T; done: boolean; }> | undefined; }>; } & RemotableBrand<...>' is not assignable to type 'ConsistentAsyncIterable<T>'.
  The types returned by '[Symbol.asyncIterator]().next(...)' are incompatible between these types.
    Type 'Promise<{ value: T; done: boolean; }> | undefined' is not assignable to type 'Promise<IteratorResult<T, T>>'.
      Type 'undefined' is not assignable to type 'Promise<IteratorResult<T, T>>'.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I think I know. TS doesn't know that the chained calls, and in particular .then() doesn't execute the callbacks synchronously, so it can't know that myIterationResultP = undefined; hasn't happened.

@mhofman
Copy link
Member

mhofman commented Oct 25, 2022

I also would like to understand why that didn't cause any test to fail.

Oh thankfully it did!

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/runner/work/agoric-sdk/agoric-sdk/agoric-sdk/node_modules/@agoric/swingset-vat/src/vats/timer/types.js' imported from /home/runner/work/agoric-sdk/agoric-sdk/agoric-sdk/packages/zoe/src/contractSupport/priceAuthority.js

@turadg turadg force-pushed the ta/fewer-maxNodeModuleJsDepth branch from 17e91dc to 4551371 Compare October 25, 2022 20:28
@turadg turadg requested a review from mhofman October 25, 2022 20:30
mhofman
mhofman approved these changes Oct 25, 2022
View reviewed changes
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.

LGTM. Thanks for fixing these types! One step closer to less ambient issues.

packages/notifier/src/asyncIterableAdaptor.js
@@ -80,6 +80,8 @@ export const makeAsyncIterableFromNotifier = notifierP => {
return harden({ value, done });
});
}
// xxx hint for type checker
assert(myIterationResultP);
Copy link
Member

Choose a reason for hiding this comment

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

Oh I think I know. TS doesn't know that the chained calls, and in particular .then() doesn't execute the callbacks synchronously, so it can't know that myIterationResultP = undefined; hasn't happened.

@turadg turadg added the automerge:no-update (expert!) Automatically merge without updates label Oct 25, 2022
@turadg turadg changed the title fewer maxNodeModuleJsDepth no more maxNodeModuleJsDepth Oct 25, 2022
@mergify mergify bot merged commit 9cc0900 into master Oct 25, 2022
@mergify mergify bot deleted the ta/fewer-maxNodeModuleJsDepth branch October 25, 2022 21:58
@turadg turadg mentioned this pull request Oct 25, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhofman mhofman mhofman approved these changes

@kriskowal kriskowal kriskowal approved these changes

Assignees
No one assigned
Labels
automerge:no-update (expert!) Automatically merge without updates
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@turadg @mhofman @kriskowal