[internal] Use State.guard in ModuleImpl #4641
Conversation
seldridge
added
the
No Release Notes
| blockStack = Nil, | ||
| blockStack = Builder.blockStack, |
There was a problem hiding this comment.
This was a little surprising to me, but the old behavior is to leave the block stack in place and let the new module see it. This seems wrong, however, this is how it works.
I'm envisioning a larger refactor of this which makes a lot of this more straightforward. I.e., a module should have a single block created inside it. It shouldn't see the parent block stack. I haven't followed through on the ramifications of this change.
There was a problem hiding this comment.
Yep! It's weird for sure. Thanks for straightening this all out, I know I'd be happy to see the sort of "order" you're proposing brought to these bits. But what you have already sure helps clean things up a lot, woohoo!
I can point you to further related bits (like Placeholder -> apply methods) if you'd like re:ramifications, but also you seem to be cruising so maybe best to just keep on 👍 . Nothing that won't fall out, I don't think.
seldridge
force-pushed
the
dev/seldridge/use-common-state-guard-in-builder
branch
from
e8c1330 to
c8a9b09
Compare
seldridge
force-pushed
the
dev/seldridge/placeholder-api
branch
from
897c742 to
a018a54
Compare
Change the way that `Module.apply` works to use the new `State.guard` method that automates the process of changing the "append point" for where commands will be added. Signed-off-by: Schuyler Eldridge <[email protected]>
seldridge
force-pushed
the
dev/seldridge/use-common-state-guard-in-builder
branch
from
c8a9b09 to
95024c0
Compare
Change code for building a
Modulefrom using one-off state save/restore code to use new, common code for this. This usesBuilder.Statemethods which were developed for thePlaceholderAPI, but were observed by @jackkoenig to be reusable here.