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

Refactor state #123

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Refactor state #123

wants to merge 2 commits into from

Conversation

bkeepers
Copy link
Contributor

The biggest hurdle to getting Storybook working (as part of #77) is the use of global state. The editor component can currently only be initialized once due to its reliance on editor_store.

This pull request begins refactoring the state into the individual components. In general, I think state should be passed into components using properties, and propagated to parent components using events.

@isaiahdahl before I spent much more time on this, what do you think about this direction?

@bkeepers bkeepers marked this pull request as draft September 13, 2022 14:44
@isaiahdahl
Copy link
Collaborator

I'll see if I can have a dig through old commits, I did have the editor in a state like this before.

I ran into some problems with the event-driven approach in getting all the components to render in the right order, but I need to dig to come up with a concrete example.

Moving all the state into a store, made getting all the components to interact with each other a breeze and makes it super flexible to move around.

Let me see if I can dig up a branch that had things working from props / events and you can comment on that. It could have just been bad heigharchy, but it also seemed like it was going to be a headache.

@isaiahdahl
Copy link
Collaborator

isaiahdahl commented Sep 13, 2022

@bkeepers
Copy link
Contributor Author

Awesome, thanks for the context. That was exactly the direction I was heading.

I wonder if one of these would be appropriate to give us state isolated to a context:

@isaiahdahl
Copy link
Collaborator

isaiahdahl commented Sep 13, 2022

Maybe they are just simple enough that it's not a concern but seems a little unnerving at face value to rely so heavily packages that haven't been updated in a while. Just based on that looks like https://github.com/blikblum/wc-context would be the most favorable pick.

What about https://github.com/nanostores/nanostores It might be just a drop in for @stencil/store but maybe there's something they do different that would enable it to work.

nanostores leverages localstorage which I don't believe @stencil/store does, so maybe that allows it to heal itself without needing to re-initialize?

@bkeepers
Copy link
Contributor Author

Maybe they are just simple enough that it's not a concern but seems a little unnerving at face value to rely so heavily packages that haven't been updated in a while.

Agreed.

@isaiahdahl
Copy link
Collaborator

I was able to get storybook working today with the editor by adjusting the lifecycles and destroying the editorview when the component gets destroyed and switching the initial lifecycle to componentWillLoad instead of connectedCallback()

Before:

...

  connectedCallback() {
    if (!state.editorView) {
      state.editorView = new EditorView({
          state: EditorState.create({
            doc: state.input,
            extensions: state.editorExtensions
          }),
          parent:  this.host.shadowRoot,
        })
      }
    }
...

After:

...

  componentWillLoad() {
    if (!state.editorView) {
      state.editorView = new EditorView({
          state: EditorState.create({
            doc: state.input,
            extensions: state.editorExtensions
          }),
          parent:  this.host,
        })
      }
  }

  disconnectedCallback() {
    state.editorView.destroy();
    state.editorView = null;
  }

...

CleanShot 2022-11-24 at 09 37 55

Gif is from a separate branch I've been working on doing some bigger refactoring.

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.

2 participants