-
Notifications
You must be signed in to change notification settings - Fork 1k
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
sdk: export more types, add showSidebar option and vm.editor.setCurrentFile #1837
Conversation
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 context for a few changes.
sdk/src/connection.ts
Outdated
@@ -1,4 +1,4 @@ | |||
import { VM } from './VM'; | |||
import { VM } from './vm'; |
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 that's obsessive of me, but I was twitching every time I saw VM.ts
and RDC.ts
in all caps alongside all our other lowercase modules. 😛
'polymer', | ||
'typescript', | ||
'vue', | ||
] as const; |
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 array is used to warn users when they use an unsupported template type (with a console.warn
), and is also used as the source of truth for the ProjectTemplate
type:
type ProjectTemplate = typeof projectTemplates[number]; // 'angular-cli' | 'create-react-app' | 'html' | …
OpenFileOption, | ||
UiThemeOption, | ||
UiViewOption, | ||
} from './interfaces'; |
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.
New typing feature: exporting more types from src/interfaces.ts
embedProjectId, | ||
openGithubProject, | ||
openProject, | ||
openProjectId, |
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 technically the same as what we had before with
import * as StackBlitzSDK from './lib';
export default StackBlitzSDK;
EXCEPT that if we add a new export to src/lib.ts
, it won't automatically end up as part of our public API.
This forces us to be more explicit and import-then-export it from src/index.ts
. Maybe a bit verbose, but more intentional and thus safer.
* Hide the sidebar on page load. | ||
* | ||
* Users will still be able to open the sidebar by clicking one of the sidebar icons. | ||
* Hide the ActivityBar (sidebar icons). |
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.
Tentatively fixed the description for the hideExplorer
option.
It's hard to describe in non-jargony terms, and I'm guessing we added this arcane feature for some customer at one point that didn't want the sidebar or ActivityBar to show at all. This may become obsolete if we change the embed layout to use a drawer menu like the main layout does on small screen (either all the time in embeds, or only on small screens).
*/ | ||
hideNavigation?: boolean; | ||
showSidebar?: boolean; |
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 one is a bit hard to describe because it signals an intent BUT we may not respect it in some cases.
Especially on small screen widths (<1000px), we would hide the sidebar by default even if receiving showSidebar=1
. I didn't describe the full implementation (which we're also tweaking right now), but used a comment that suggests we might do something different on small screens.
openFile: (value: Options['openFile']) => stringParams('file', value).join('&'), | ||
theme: (value: Options['theme']) => enumParam('theme', ['light', 'dark'], value), | ||
view: (value: Options['view']) => enumParam('view', ['preview', 'editor'], value), | ||
}; |
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.
Refactored how we generate the URL query string.
This is a bit more indirect than what we had before (a single function going through each option with code like if (options.someOption) params.push('someOption='+options.someOption)
), but it has two upsides:
- We can reuse code used to validate parameter values depending on their type.
- Using the
Record<keyof Options, …>
type, we make sure that we are handling all supported options and not forgetting one. When we add an option, TS forces us to add a generator or exclude that option name intype Options = Omit<…>
.
type: 'SDK_GET_PREVIEW_URL', | ||
payload: {}, | ||
}) | ||
.then((data) => data?.url ?? 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.
Fix for the issue where we were returning { url: string } | null
and not string | null
.
@@ -153,6 +161,9 @@ export class VM { | |||
* @experimental | |||
*/ | |||
setUrl: (path: string = '/'): Promise<null> => { | |||
if (typeof path !== 'string' || !path.startsWith('/')) { | |||
throw new Error(`Invalid argument: expected a path starting with '/', got '${path}'`); | |||
} |
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.
Having this method called setUrl
when you can only pass a path and not a full URL is a bit misleading. The argument being called path
was not enough to avoid one beta tester from trying to pass a full URL.
There are two possible fixes:
- Change the method name to
setPath
orsetUrlPath
. (But we kinda like having matchinggetUrl
andsetUrl
methods. I'd be open tosetUrlPath
, if we think that's a better choice.) - Or keep the method name as
setUrl
, and validate the input.
I went with the second option right now, but we can reconsider.
3ef777a
to
0e28409
Compare
* @since 1.7.0 | ||
* @experimental | ||
*/ | ||
setCurrentFile: (path: string): Promise<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.
New feature that is useful for multi-step demos with more than one file open at a time (e.g. 2 files in 2 side-by-side panes).
In a multi-step demo, it can be useful to explicitly set the currently focused file out of all the files that are open in tabs, so that:
- That file gets revealed in the file explorer sidebar (e.g. expanding the folder hierarchy to reveal that file).
- And to make sure that the file's tab gets the "open" state, if it's one of several tabs in a pane.
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.
Right now I'm erring on the side of not opening an editor tab for the file if there isn't one already. Why? Because that would raise complex questions that we could only answer with questionable defaults.
Let's say there are open tabs (and maybe several panes) already:
- Do we keep the existing tabs and just open a new one? Hmm probably.
- Okay but what if at least one of the existing tabs is in "preview" mode (which will happen when using
openFile
to open one or several files initially): do we replace one of the tabs then? (Technically we should, but that might be an unexpected result!) - And if we have several panes, where do we add the new tab? First pane? The pane which hosts the currently focused editor tab (if any)? Will that make the result unpredictable, depending on user actions?
In a multi-step demo, it's probably better for embedders to explicitly call:
// Reset the open tabs, even if that was the state used in the previous demo step,
// because the user may have interacted with the editor and closed some tabs!
await vm.editor.openFile(['src/foo.js', 'src/bar.js']);
// Select the right-side file
await vm.editor.setCurrentFile('src/bar.js');
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 for implementing these features and helping with all types ✨
@@ -22,62 +21,14 @@ export function embedUrl(route: string, options?: EmbedOptions) { | |||
if (options && typeof options === 'object') { | |||
Object.assign(config, options); | |||
} | |||
return `${getOrigin(config)}${route}${buildProjectQuery(config)}`; | |||
return `${getOrigin(config)}${route}${buildParams(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.
Thanks for refactoring this!
…ntFile (#1837) * fix vm.editor.getUrl returning an object and not a string * improve types of the RDC class * export all types from src/interfaces.ts * refactor URL param generation * add showSidebar option * sdk: add experimental vm.editor.setCurrentFile * bump version to 1.7.0-alpha.3
Grab-bag of improvements for the upcoming 1.7.0 SDK release.
vm.editor.getUrl
return value (we usedPromise<string | null>
but were returningPromise<{ url: string } | null>
).showSidebar: boolean
option, translating to the URL parametershowSidebar=1
andshowSidebar=0
.vm.editor.showSidebar(boolean)
method that we already support.vm.editor.setCurrentFile
method.src/interfaces.ts
instead only a select few.EmbedOptions['view']
didn't return the type they wanted, since it returnsUiViewOption | undefined
instead ofUiViewOption
, and you have to use TypeScript shenanigans on top of that to remove theundefined
part.UiView
andUiTheme
toUiViewOption
andUiThemeOption
. Since exporting those types makes them a public API, so that any change to those types would be a BREAKING change, I wanted to get the names explicit enough, even if that means a bit more verbosity.src/rdc.ts
andsrc/vm.ts
.