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

Saving environment across the full course #24

Open
hfboyce opened this issue Mar 9, 2020 · 3 comments
Open

Saving environment across the full course #24

hfboyce opened this issue Mar 9, 2020 · 3 comments

Comments

@hfboyce
Copy link
Contributor

hfboyce commented Mar 9, 2020

Sorry @ines for referencing this again,
I tried changing isolateCells: true in src/components/juniper.js and I am still not having follow up cells retaining the variables in the previous cell's environment.
Is there anywhere I should look to debug this? I think this feature could be really useful especially for importing modules and data.

Originally posted by @hfboyce in #13 (comment)

@ines
Copy link
Owner

ines commented Mar 11, 2020

Thanks for opening this as a separate issue – I missed that other comment!

Are your code cells spread out across multiple exercises? And does it make a difference if you try it with two code blocks in the same exercise? If it does work within the same exercise, that'd indicate that the problem is the component mounting/unmounting. When the <Juniper /> component remounts, it has essentially lost this.state.kernel here and will reconnect regardless.

If this is the case, one solution could be to store the kernel object outside of the component, maybe via React Context? The idea would be that the index.js keeps the kernel object state, not the Juniper component:

const [kernel, setKernel] = useState(null)

This is the source of truth that gets passed down to all code cells (and gets updated after it connects or if the kernel happens to die in between). Using context here means that we don't have to pass it to every code celll as a prop.

In context.js, we could create another context for the kernel:

export const KernelContext = React.createContext(null);

And then wrapp the whole app in <KernelKontext.Provider value={{ kernel, setKernel }}>. The CodeBlock component could then consume that context and have access to the kernel and setter (it's a class component, so we need to use the slightly uglier consumer component here):

render() {
    // etc.
    return (
        <KernelContext.Consumer>
          {({ kernel, setKernel }) => /* rest of the CodeBlock component, starting with <StaticQuery> etc. */}
        </KernelContext.Consumer>
    )
}

We can then add two more props to the <Juniper> component: kernel and setKernel and replace all references to this.state.kernel with this.props.kernel, and all calls to setState that change the kernel with calls to setKernel.

Even as code blocks mount and unmount, the kernel will stay available, as it's state stored on the app. I haven't actually tried this and it's possible that I'm wrong about something here... but I thought I'd write down my ideas in case you want to give it a go (since I probably won't have time to look at it in detail this week) 🙂

@hfboyce
Copy link
Contributor Author

hfboyce commented Mar 11, 2020

Hi @ines,
Thank you so much for getting back to me! 🙏

Are your code cells spread out across multiple exercises?

Yes but ....

Does it make a difference if you try it with two code blocks in the same exercise?

When I test with 2 code cells in the same exercise, my variables and packages still don't get recognized.

I am not sure if that will change your suggestion but I'll still give it a go!

Developing is something that I am still learning so i'll see if I can play around without burning my course 😂.

@ines
Copy link
Owner

ines commented Mar 12, 2020

When I test with 2 code cells in the same exercise, my variables and packages still don't recognized.

Ahhhhh, so I think I came to a wrong conclusion here, but thinking about it some more, the issue here is probably still what I thought it might be and related to how React renders the components.

In my standalone Juniper code, only one instance of Juniper is created that holds the kernel (this.kernel) and references to all the code cells that are runnable. So it's able to reuse that kernel.

However, in this implementation here, the code block components are created with their own instance of Juniper, each with its own kernel object instance (this.state.kernel). So the kernels can't be reused across cells. If we only had one kernel per app and pass that down to each code block, that should make it reusable.

Developing is something that I am still learning so i'll see if I can play around without burning my course 😂.

Sure and please don't feel any pressure to implement any of this 😅 I just thought I'd share my idea, in case it works and you (or someone else) want to try it out.

If you do end up playing with it and haven't used that React context stuff before, this post has a nice explanation. Basically, the problem it solves is that there are many code blocks that are created programmatically and that are several levels down the component tree. But they all need that kernel value and a callback function to update it (if no kernel is available yet, or if it died). So instead of passing it all the way down, we make it available as context and allow the code blocks to "consume" it.

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

No branches or pull requests

2 participants