-
Notifications
You must be signed in to change notification settings - Fork 131
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: svelte integration #1748
base: main
Are you sure you want to change the base?
feat: svelte integration #1748
Conversation
|
053c26a
to
3f8c7dc
Compare
Preview deployed to https://412bf31d-8d95-47bc-968a-16ed2c485436--scalar-deploy-preview.netlify.app |
#1807 Bundle Size — 1.92MiB (0%).Warning Bundle contains 4 duplicate packages – View duplicate packages Bundle metrics
|
Current #1807 |
Baseline #1800 |
|
---|---|---|
Initial JS | 1.92MiB |
1.92MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 0% |
100% |
Chunks | 1 |
1 |
Assets | 1 |
1 |
Modules | 1085 |
1085 |
Duplicate Modules | 0 |
0 |
Duplicate Code | 0% |
0% |
Packages | 157 |
157 |
Duplicate Packages | 4 |
4 |
Bundle analysis report Branch anthony/doc-1832-svelte-integrat... Project dashboard
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.
Overall a solid start for an integration.
const loadCdnScript = () => { | ||
const cdnScript = document.createElement('script') | ||
cdnScript.src = "https://cdn.jsdelivr.net/npm/@scalar/api-reference" | ||
document.body.appendChild(cdnScript) | ||
} |
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.
Uh, pardon me but why are you loading a script from CDN? Why not use the esm
provided by @scalar
?
Take a look at the react implementation
import { type ClientRequestConfig, useRequestStore } from '@scalar/api-client' |
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.
good call! depending on packages both approaches are taken, i took this shortcut as a starter however your are right, will update accordingly 🙏
validateSpec(spec) | ||
|
||
contentString = await getContentString(spec) | ||
applyDefaultCss(config) |
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.
Should this not be in onMount()
?
const { spec, ...config } = configuration | ||
const apiScript = document.getElementById('api-reference') | ||
|
||
validateSpec(spec) |
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.
Should this not be in onMount() ?
|
||
const initializeApiReference = async () => { | ||
const { spec, ...config } = configuration | ||
const apiScript = document.getElementById('api-reference') |
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.
You should use svelte:bind
<div> | ||
<script id="api-reference" type="application/json"></script> | ||
</div> |
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.
Rendering it like this means we get no server side rendering (SSR)
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.
some work is in progress on the api reference package in order to avoid some issue here for ssr, once done i'll jump back here!
But there's another problem i see, your integration method is not officially supported. Use @sveltejs/kit |
thanks @baseplate-admin for the solid reviews! i started to dive into a better approach method with @Svelte-kit |
you are welcome, feel free to ask me for anything. also do you know that svelte itself is moving from version 4 to version 5. Please make sure that this patch works with both version 4 and 5. |
3f8c7dc
to
bb0ab01
Compare
bb0ab01
to
8adeb8b
Compare
this pr is a draft proposal to support a svelte integration as requested by @baseplate-admin