-
Notifications
You must be signed in to change notification settings - Fork 243
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
VSCode extension: add openapi3 preview #6015
base: main
Are you sure you want to change the base?
Conversation
# Conflicts: # packages/typespec-vscode/package.json # packages/typespec-vscode/src/extension.ts # packages/typespec-vscode/src/types.ts # pnpm-lock.yaml
# Conflicts: # pnpm-lock.yaml
# Conflicts: # pnpm-lock.yaml
I have to copy |
All changed packages have been documented.
|
You can try these changes here
|
https://github.com/arjun-g/vs-swagger-viewer is not suitable as a dependent extension for previewing swagger, because it only exposes a load method, which doesn't fulfill our requirement:
|
List of changes since last review:
|
List of changes since last review:
|
@@ -27,6 +28,24 @@ export async function isDirectory(path: string) { | |||
} | |||
} | |||
|
|||
export async function createTempDir(): Promise<string | undefined> { |
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.
node has mkdtemp
which does that too
return; | ||
} | ||
|
||
const mainTspFile = selectedFile.endsWith("main.tsp") ? selectedFile : await getMainTspFile(); |
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 the same issue as #6062, if the user asked for a preview of a file it should respect that and not try to fetch a main.tsp
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.
when I discussed this feature with Rodge, we thought we should show the full open api 3 spec v.s. just the selected spec. Sometimes, showing the selected spec may be meaningless. For example, client.tsp
is for client codes, so it should not have impact on the Open API 3 output which is for service API.
@RodgeFu Your thought?
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.
Yeah but it's not up to you to decide that for the user. Typespec compile takes any file as entry point it just default to a file called main.tsp when you target a directory compile .
Client.tsp is not the only use case. At the very least there should be a choice the user can make.
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 this is about how the files in project can be compiled properly. I believe in most case, starting compilation from "main.tsp" is still the right way to go and simply using the file user selected as entrypoint is likely not to compile the project as expected. So I think we should keep the "main.tsp" as the default behavior while providing some way for people to have the choice when needed.
Some options I can think about:
- provide a settings for user to set entrypoint file when needed
- adding a quickpick step for people to select main.tsp (default) or the selected file when user clicks preview
Actually the first one feels like something should be in tspconfig.yaml instead of vscode. @timotheeguerin , do we have any plan for it there?
open to any better ideas.
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 there is if there is a exports
or tspMain
(deprecated) in package.json but nothing to have sub project per package. Having many project per package is still quite a common use case(whole of the specs repos)
Maybe we should investigate also providing an entrypoint from the config but the way it works today is the other way around it resolve a config for an entrypoint which many can share
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.
yeah, I think it will be useful if user can define the entrypoint in the config when people using tspconfig.yaml per project instead of package.json like the spec repo.
As for this PR, considering we are already able to config the entrypoint in package.json for user to customize now, I think we can try to check package.json file with tspMain/exports configured first. if found, then start compilation from there (just pass the folder and cli should be able to handle it). Otherwise, fallback to trying to find main.tsp. Later on, when we support entrypoint in tspconfig.yaml, we can add that part.
And about the exports
and tspMain
in package.json, I checked the code to resolve entrypoint for compile and found it only checks tspMain
in the package.json but nothing about exports
. Also I can only find exports
and tspMain
are mentioned for Package import in the doc. Could you help to provide more detail about the exports
as entrypoint of compile? (
const tspMain = resolveTspMain(pkg); |
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 are right it doesn't do that now, but this is a bug, it only resolve exports
when doing imports or emitter names.
This is a bug because the main purpose of this is to be able to build your library (where you might just have exports
now and this means you cannot do tsp compile .
in your library package)
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.
Filed #6219
} | ||
|
||
async function getOutputFolder(mainTspFile: string): Promise<string | undefined> { | ||
let tmpFolder = openApi3TempFolders.get(mainTspFile); |
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.
when does caching become useful here?
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.
- When a tsp file is changed, the file watcher will re-generate open api 3 output and reload the preview panel. At that time, if the corresponding temp folder is already created, it will re-use that folder.
- When the preview panel is closed and opened again, the same logic will be triggered.
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.
Do you clear the dir in between, if the user goes change the name of services or version it could create ghost files otherwise
vsce package
not supporting pnpm, I have to copyswagger-ui-dist
lib files into our source folder. So, add those files into ignore list of lint and format.resolve #4858