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

chore: Bump codemirror #1148

Merged
merged 8 commits into from
Jul 4, 2024
Merged

chore: Bump codemirror #1148

merged 8 commits into from
Jul 4, 2024

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Jun 18, 2024

Bumps CodeMirror up to v6, somewhat pulled from #793 (man was that PR a gold mine of stuff). Bit of an increase on the bundle size (loading the 'Simple Counter' w/ no cache: 393kb -> 448kb, -12kb for the uncompressed favicon in preview, or ~43kb bump), but I don't think it's too bad.

I've enabled auto completion (basic, but not bad) and bracket matching with this bump, seem like general nice-to-haves and I am already quite happy with these. There seems to be quite a bit more we can also enable/play around with in the future too.

Comment on lines +15 to +16
// Custom theme that better matches our Prism config, though
// the lexer is somewhat limited so it still deviates
Copy link
Member Author

@rschristian rschristian Jun 18, 2024

Choose a reason for hiding this comment

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

Theme doesn't quite match our Prism config perfectly, but it's a better match than the previous was.

There was a heck of a lot of gold/yellow highlighting in the previous theme.

@rschristian rschristian force-pushed the chore/bump-codemirror branch 4 times, most recently from 9850f20 to 9c64641 Compare June 19, 2024 03:50
@rschristian rschristian force-pushed the chore/bump-codemirror branch 2 times, most recently from 24a21c8 to e9e06c7 Compare June 19, 2024 19:41
@rschristian rschristian force-pushed the chore/bump-codemirror branch 3 times, most recently from 43ca72d to eae5751 Compare June 29, 2024 21:10
Comment on lines +17 to +27
const highlightStyle = HighlightStyle.define([
{ tag: tags.keyword, class: 'cm-keyword' },
{ tag: [tags.definition(tags.function(tags.name)), tags.function(tags.name), tags.propertyName], class: 'cm-function' },
{ tag: tags.literal, class: 'cm-literal' },
{ tag: tags.tagName, class: 'cm-tag' },
{ tag: tags.attributeName, class: 'cm-attribute' },
{ tag: tags.string, class: 'cm-string' },
{ tag: [tags.operator], class: 'cm-operator' },
{ tag: tags.comment, class: 'cm-comment' },
{ tag: tags.invalid, class: 'cm-invalid' }
]);
Copy link
Member Author

@rschristian rschristian Jun 19, 2024

Choose a reason for hiding this comment

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

I checked some of our code snippets and everything seems to be covered, but let me know if anyone spots anything that is missing or off looking.

Comment on lines +43 to +45
useEffect(() => {
if (props.slug || !editor.current) routeHasChanged.current = true;
}, [props.slug]);
Copy link
Member Author

@rschristian rschristian Jun 30, 2024

Choose a reason for hiding this comment

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

I'm quite convinced preact-iso is problematic in its behavior here.

Unfortunately, it updates the route context first-thing, passing the new path, params, etc. as soon as it has found a matching route. This bites us pretty badly here as while we need to watch for route changes, the corresponding content for that route change will always come in separately and later.

Unless someone's got a better idea (and it's very possible I'm missing something obvious, I've been beating my head against this for way more hours than I'd like to admit to), I think we need something along these lines here for now.

We need some way of differentiating w/ iso, in components, a new incoming route to load new data and a finished route change (here we'd watch the later). This will probably be a breaking change and will require unraveling the router a fair bit, so on my ever-growing TODO list it goes.

Comment on lines -24 to -41
this.showErrorTimer = setTimeout(() => {
this.editor.operation(() => {
let { left } = this.editor.cursorCoords(
{ line: error.loc.line - 1, ch: error.loc.column - 1 },
'local'
);
let ref;
const errorLine = (
<div ref={r => (ref = r)} class={style.lintError}>
<pre style={`padding-left:${left}px;`}>▲</pre>
<div>🔥 {error.message.split('\n')[0]}</div>
</div>
);
render(errorLine, this.scratch);
this.errors = [this.editor.addLineWidget(error.loc.line - 1, ref)];
});
}, 1000);
}
Copy link
Member Author

@rschristian rschristian Jun 30, 2024

Choose a reason for hiding this comment

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

Errors in the editor have been broken for a fair while now, I think it was #971? The error stack in the runner pane is completely void of positioning info which, I'm guessing, leads into a lack of editor errors as we can't place them.

I'll look at re-adding this soon, once I fix the error stack traces.

Edit: Tracking in #1158

Copy link
Member Author

@rschristian rschristian Jul 1, 2024

Choose a reason for hiding this comment

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

Actually, the tutorial does seem to show errors occasionally. Will need to take another look.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error positioning is pretty touchy & limited, we can't effectively map the built output back to the code editor input.

Redoing the compilation is a can of worms I don't want to address here. For now, I've disabled the error overlay in the editor as it barely works and tends to point at the wrong line entirely.

@rschristian rschristian marked this pull request as ready for review June 30, 2024 04:03
@rschristian rschristian marked this pull request as draft June 30, 2024 04:05
@rschristian rschristian force-pushed the chore/bump-codemirror branch 6 times, most recently from b2965d4 to 7bfc449 Compare June 30, 2024 20:16
@rschristian rschristian marked this pull request as ready for review July 4, 2024 00:01
@rschristian rschristian merged commit 4ac8c9d into master Jul 4, 2024
5 checks passed
@rschristian rschristian deleted the chore/bump-codemirror branch July 4, 2024 07:56
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