-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add svelte 3 support #1820
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
Add svelte 3 support #1820
Conversation
CompuIves
left a comment
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 reaaally good! Some nitpicks, but perfectly implemented! Would be good to do a bit of backwards compat testing.
packages/app/src/sandbox/eval/transpilers/svelte/svelte-worker.js
Outdated
Show resolved
Hide resolved
packages/app/src/sandbox/eval/transpilers/svelte/svelte-worker.js
Outdated
Show resolved
Hide resolved
packages/app/src/sandbox/eval/transpilers/svelte/svelte-worker.js
Outdated
Show resolved
Hide resolved
packages/app/src/app/components/CodeEditor/VSCode/Configuration/index.tsx
Show resolved
Hide resolved
- Also send warnings from compiler - Save warnings in cache and reuse them - Optimize the VSCode svelte extension
packages/app/src/sandbox/eval/transpilers/svelte/svelte-worker.js
Outdated
Show resolved
Hide resolved
standalone-packages/codesandbox-browserfs/src/generic/file_index.ts
Outdated
Show resolved
Hide resolved
…ex.ts Co-Authored-By: SaraVieira <[email protected]>
| function getV3Code(code, path) { | ||
| self.importScripts(['https://unpkg.com/[email protected]/compiler.js']); | ||
| try { | ||
| const { js, warnings } = self.svelte.compile(code, { |
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.
could we consider something like this?
self.importScripts(['https://unpkg.com/[email protected]']);
try {
const explorer = cosmiconfig('svelte');
const {config} = await explorer.search('svelte');
if (config) ({code} = self.svelte.preprocess(code, config.preprocess));
const { js, warnings } = self.svelte.compile(code, {
// ...user-side code:
svelte.config.js
const ts = require('svelte-ts-preprocess')
const sass = require('svelte-preprocess-sass')
module.exports = {
preprocess: [ts, sass, require('./custom-preprocessor')]
}I'm not sure that cosmiconfig will work in the browser, and maybe there are better approaches to load the config with preprocessors for codesandobx. But this kind of addition will help users set their custom svelte preprocessors (like https://github.com/ls-age/svelte-preprocess-sass or https://github.com/PaulMaly/svelte-ts-preprocess)
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.
Cosmic config doesn't seem to work in the browser :/
https://codesandbox.io/s/nnn2lmnkqm
This is an option we will do but I think we will need to check for the file ourselfs and get the data out
I am also not sure how these preprocessors will work in the browser. We have our own preprocessors for ts and sass and it would be great if we could use them :(
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.
It miiiight maybe work in the worker, we mock fs there and some other utils. But there is also a valid chance that it won't work. Especially since I do this 😅 : https://github.com/CompuIves/codesandbox-client/blob/b75e9969baa73478b05f49e7d8da85b08945f5ad/packages/app/config/polyfills.js#L14
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 think it makes sense to keep the simple option (maybe add some hardcoded support for sass/others) in the browser and give the option to move to a container if there is advanced configuration needed.
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.
maybe, besides the hardcoded support for sass and other preprocessors, it would be useful to hardcode something like require('/svelte.config.js') too? to provide an option to also add some custom preprocessors by a user. and that would be the user's responsibility, how that preprocessors work in the browser
like,
let config;
try {
config = require('/svelte.config.js');
} catch (e) {}
try {
if (config) ({code} = self.svelte.preprocess(code, codesandboxPreprocessors.concat(config.preprocess || [])));
const { js, warnings } = self.svelte.compile(code, {
// ...|
YAY! |
* add svelte3 first try * try again * add svelte extension; update logo * Don't compile svelte files * ts update * add icon * add SOME error handling * fix eslint? * Update the svelte vscode extension to have LSP * fixes from pr review * fix older verwsins * Svelte Improvements - Also send warnings from compiler - Save warnings in cache and reuse them - Optimize the VSCode svelte extension * Put v3 code generation on top * Update standalone-packages/codesandbox-browserfs/src/generic/file_index.ts Co-Authored-By: SaraVieira <[email protected]> * add version automacially from installed version * make svelte app downloadable and deployable
Checlist:
Testing sandbox: http://pr1820.cs.lbogdan.tk/s/4r00193j37
closes #1809
Also closes #1765 as I updated the sass loader