ines / course-starter-python

👩‍🏫🐍 Starter repo for building interactive Python courses
https://course-starter-python.netlify.com
MIT License
504 stars 118 forks source link

Saving environment across the full course #24

Open hfboyce opened 4 years ago

hfboyce commented 4 years ago

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 https://github.com/ines/course-starter-python/issues/13#issuecomment-593548050

ines commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.