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

refactor: move around some helpers before state-sync #7258

Merged
merged 4 commits into from
Mar 30, 2023
Merged

refactor: move around some helpers before state-sync #7258

merged 4 commits into from
Mar 30, 2023

Conversation

mhofman
Copy link
Member

@mhofman mhofman commented Mar 28, 2023 •
edited
Loading

refs: #3769

Description

This extracts a few pure refactor commits from #7225 in an attempt to make that PR more digestible.

Security Considerations

None

Scaling Considerations

None

Documentation Considerations

None

Testing Considerations

None

@mhofman mhofman requested a review from michaelfig March 28, 2023 21:15
@mhofman mhofman added the automerge:rebase Automatically rebase updates, then merge label Mar 28, 2023
@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from 96fd4ae to aa2fa8d Compare March 28, 2023 21:18
@mhofman
Copy link
Member Author

mhofman commented Mar 28, 2023

Sorry for the temporary churn, I mistakenly cherry-picked the wrong commit and pressed push too quickly.

michaelfig
michaelfig reviewed Mar 28, 2023
View reviewed changes
packages/internal/src/shutdown.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what to think about this. Part of me agrees that we can have platform dependencies in @agoric/internal if we don't export those modules from '@agoric/internal', rather by using a deep import instead. But in practice, those platform dependencies are awfully hard to diagnose if an endo-bundled module accidentally imports one by contagion.

Could you instead move the Node.js dependents to internal/src/node/*, so at least it is really obvious when we import them or decide to find them a new home?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done for shutdown and other helpers relying on node. PTAL

@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from aa2fa8d to 3eb54d5 Compare March 29, 2023 00:59
@mhofman mhofman requested a review from michaelfig March 29, 2023 01:01
michaelfig
michaelfig approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

LGTM! Onward and upward....

@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from 3eb54d5 to bd8896e Compare March 29, 2023 05:02
@mhofman
Copy link
Member Author

mhofman commented Mar 29, 2023

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2023

requeue

✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically

@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch 2 times, most recently from 2440551 to 9607757 Compare March 29, 2023 20:15
@mhofman mhofman removed the automerge:rebase Automatically rebase updates, then merge label Mar 30, 2023
@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from 9607757 to e6e4c8e Compare March 30, 2023 14:52
@mhofman mhofman added the force:integration Force integration tests to run on PR label Mar 30, 2023
@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from e6e4c8e to 136f56d Compare March 30, 2023 19:02
@mhofman mhofman added automerge:rebase Automatically rebase updates, then merge and removed force:integration Force integration tests to run on PR labels Mar 30, 2023
@mhofman mhofman force-pushed the mhofman/state-sync-refactor branch from 136f56d to 519d707 Compare March 30, 2023 19:35
@mergify mergify bot merged commit 037fad8 into master Mar 30, 2023
@mergify mergify bot deleted the mhofman/state-sync-refactor branch March 30, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelfig michaelfig michaelfig approved these changes

Assignees
No one assigned
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mhofman @michaelfig