-
Notifications
You must be signed in to change notification settings - Fork 366
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
feat(codeeditor): support additional, user-supplied language modes #570
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5bffe07:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jodyheavener, for taking this issue! I'm glad it works, and I just left some comments about how you're consuming the props, great job!
sandpack-react/src/components/CodeEditor/CodeMirror.stories.tsx
Outdated
Show resolved
Hide resolved
| "html" | ||
| "vue" | ||
| "markdown"; | ||
fileType?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can extend this key using generics and still keep type-safe, let me know if you need any help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been wracking my brain trying to make this work, but I'm not sure I'm fully understanding how we can use generics in this case. Would love some guidance if you have a chance!
] | ||
`; | ||
|
||
exports[`useSyntaxHighlight renders a shell block 1`] = `null`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting why the output is null, could you take a look at this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is odd - it looks like it's stumbling on the provided code sample? If I change the mocked "shell" code to the Python code it works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was exploring this a bit more, and it looks like this occurs with Markdown as well: https://github.com/codesandbox/sandpack/blob/main/sandpack-react/src/components/CodeEditor/__snapshots__/useSyntaxHighlight.test.tsx.snap#L1457
@@ -123,13 +123,13 @@ export const CustomLanguagePython: React.FC = () => ( | |||
<CodeEditor | |||
additionalLanguages={[ | |||
{ | |||
name: "python", | |||
name: "python" as const, // TODO: how to convert it to a union type instead of a string? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a cool approach! Does it need to be a union, since it would never be concerned about existing file types?
What kind of change does this pull request introduce?
Feature
What is the current behavior?
Sandpack language support is currently limited to the built-in languages. This means that you cannot currently use Sandpack with code in other languages correctly (it would default to Markdown).
What is the new behavior?
This PR adds the ability to add custom language modes by supplying a language type, applicable file extensions, and a
LanguageSupport
instance for that syntax mode.The
CodeEditor
component now takes anadditionalLanguages
prop of the following type:(The
Sandpack
preset component also takes this in via itsoptions.codeEditor
prop object and passes it down toCodeEditor
.)This new prop value is pulled out of the context in
CodeMirror
and passed down to thegetLanguageFromFile
to establish the language name, andgetCodeMirrorLanguage
to extract the custom language support and pass it over to CodeMirror like our other language extensions.What steps did you take to test this? This is required before we can merge, make sure to test the flow you've updated.
The following Storybook entries show off the new functionality:
/story/presets-sandpack-options--custom-languages
shows off the usage of multiple custom languages in a preset/story/components-codemirror--custom-language-shell
shows off the usage of the custom Shell language/story/components-codemirror--custom-language-python
shows off the usage of the custom Python languageChecklist
Additional comments
This is mostly a proof of concept, and I welcome any larger structural feedback! I also have not written tests yet; I will do so if this approach seems correct.
One thing I have noticed is that this approach requires the loosening of language types, specifically
CodeMirrorProps["fileType"]
andgetLanguageFromFile
's return type ofSandpackLanguageSupport
. I believe this is necessary for the support of custom languages, but it's something to keep in mind when reviewing - perhaps there is a better approach.