Skip to content

overhaul adapter code #249

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

Merged
merged 28 commits into from
Mar 13, 2023
Merged

overhaul adapter code #249

merged 28 commits into from
Mar 13, 2023

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Mar 10, 2023

This simplifies the adapter code per #248. There was a bit too much indirection previously (also we were conflating state and events, which I think is a bit of a code smell) and I found it quite hard to make changes — there's still room for improvement but I'm much happier with this.

There are a couple of glitches to work through still:

@vercel
Copy link

vercel bot commented Mar 10, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
learn-svelte-dev ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 11, 2023 at 8:09PM (UTC)

/** @type {Promise<boolean>} */
let current;
let token = {};
async function next() {
Copy link
Member

Choose a reason for hiding this comment

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

The simplification inside Output.svelte is great, but I think we need to keep this queue implementation. The new code is - I think - prone to race conditions when typing fast, toggling between solution and initial state fast etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't observed that to be the case, but if it is then I think we want to implement the queuing logic in webcontainer/index.js, where there's already an await running (which appears to only be used in the reset case) — having multiple layers was part of the reason the code was hard to work with. As far as adapter.js goes it should be fire-and-forget, I think

@Rich-Harris
Copy link
Member Author

still occasionally encountering errors when navigating between apps, but no more than currently, so i'll merge this to unblock other work

@Rich-Harris Rich-Harris merged commit 0aea741 into main Mar 13, 2023
@Rich-Harris Rich-Harris deleted the gh-248 branch March 13, 2023 16:14
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