-
Notifications
You must be signed in to change notification settings - Fork 61
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 migration #124
Vscode migration #124
Conversation
Current status: working great! TODO before acceptance:
|
b1173fc
to
0f47d16
Compare
@bhufmann For now, it only runs as a vscode-plugin (f5 from vscode after running yarn build everywhere), I tried to add the browser and electron apps, but even though the package.json file and the directory are identical to what is there now, except that the dependency to the theia-trace-extension is removed, it doesn't build! But maybe if we release the vscode extension soon, we won't be needing those. |
Since this is a big patch whose rebases may not always be trivial, please let me know when you plan to review this, so I can rebase it on the latest master. Thanks! |
0f47d16
to
f41c54b
Compare
It's a bit hard to follow right now where things changed (not being very aware of recent changes generally). The build errors I see are probably not directly related to the vscode extension - they should be able to build independently (contrary to Theia extensions). About the vscode extension: I suggest you build and package the extension and then just add a To package, add |
}, | ||
"workspaces": [ | ||
"viewer-prototype", | ||
"browser-app", |
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.
Is the intention to drop the example applications?
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.
Add entries for the new app folders if you want them built along with the rest:
"app/browser",
"app/electron"
package.json
Outdated
"start:browser": "yarn rebuild:browser ; yarn --cwd browser-app start", | ||
"start:electron": "yarn rebuild:electron ; yarn --cwd electron-app start", | ||
"build": "lerna run build", | ||
"build:browser": "theia rebuild:browser", |
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 would advise to keep the original names: rebuild:*
- it's kind of a convention and does not build the app, just rebuilds the native npm dependencies for either browser
or electron
.
"@typescript-eslint/parser": "^2.30.0", | ||
"babel-eslint": "10.1.0", | ||
"babel-jest": "^24.9.0", | ||
"babel-loader": "8.1.0", |
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 see this dependency not being found when building the browser example app
. Maybe it should be a runtime dependency. but most likely it's not a good idea to mix Theia and vscode stuff under the same monorepo, with shared hoisted dependencies.
Since the Theia example app and the vscode trave viewer extension have no build-time relation, they should be be built separately. They can still be in the same repo though, just not in the same yarn workspace.
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 nice start @tahini and @babyadoresorange ! I did a quick high-level overview and have some minor comments below, but I will do a deeper code dive and test the extension further this week. I think it will be easier to review when some of the generated create-react-app
things are cleaned up.
@@ -0,0 +1,263 @@ | |||
{ | |||
"name": "@trace-viewer/vscode-extension", | |||
"version": "0.0.0", |
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.
Add a publisher
here for vsce package
to work. Marc alludes to this in his comment above.
@@ -0,0 +1,263 @@ | |||
{ | |||
"name": "@trace-viewer/vscode-extension", |
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 trying to package the extension with vsce package
it complains that this is an invalid extension name. I think it doesn't liked the scoped naming.
<circle cx="420.9" cy="296.5" r="45.7"/> | ||
<path d="M520.5 78.1z"/> | ||
</g> | ||
</svg> |
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 can remove this file (unless you want to use the react logo 🙂)
@keyframes App-logo-spin { | ||
from { transform: rotate(0deg); } | ||
to { transform: rotate(360deg); } | ||
} |
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.
Seems like you may be still in the process of cleaning things up, but I know create-react-app
tends to generate a lot of cruft. Might be worth going through the react-app
package and cleaning up anything unused.
Great to see so much action on the PR. I'm going to try it out as well. |
@tahini is it desired to keep the old Theia extension for now? (other than in the repo's history) Ideally the repo's structure would be simplified if the main content is a vscode extension. The example applications can still have entries in the main But it would also work to just locally clone the repo, open it in vscode and press Repos that host a vscode extensions tend to be simple and follow a similar pattern. Generally the extension is built at repo root, with the root
If this repo follows similar, we can submit a PR here to add an entry for our extension, and it will be automatically published on |
Q: I see that the vscode extension seems to be split in a few components/packages:
Are these the seeds of a group of vscode extensions that would, together, provide trace viewing capabilities? Or more the internal breakdown of components of, ultimately, a single trace viewing extension? |
I was able to run it and open a trace in VsCode. Nice! I noticed that the events table is not working properly. I tried to debug in the react-components, but the breakpoints don't hit. I see a warning message "Unbound breakpoint". However, breakpoints work in the vscode-extension package. I wonder if something is missing in the launch configuration, but I haven't figured out what. Anybody any idea? |
Thanks for the feedback. Unfortunately, the issue of debugging the webview (react components) bugs me. I don't know how to do it! It runs in an iframe and the webview developer tools of vscode does not have a debugger. Besides, our current way of building the react app builds a production app, so it wouldn't be meaningful debugging, but that can be fixed. I asked on the theia-help channel on discord and @marechal-p pointed me to https://stackoverflow.com/questions/3275816/debugging-iframes-with-chrome-developer-tools but it applies to the browser (chrome or firefox) and we are not running in the context of a real browser. Anyway, if you figure it out, let me know, but that's one thing I still have to look into. |
Hi, I had looked into debugging webviews for another extension and found this |
Thanks @danieltomasku! I found that too and I tried it. You have to be in the webview and execute that command and developer tools will open in a browser. Otherwise it won't open. Unfortunately we cannot debug in vscode itself, but browser is also ok. I was not able to setup the source maps to find the typescript source code. Any ideas how to setup the source map? |
The 'base' and 'react-components' packages are meant to be reusable components that are consumed by the vscode extension but can also be consumed by other apps, like eventual grafana or jenkins plugins. This repo is thus a monorepo that contains among other things a vscode extension. I'm not sure how this can work out with the vscode extension release. We could create another repo for the vscode extension if needed. |
I was able to get source maps working (kind of). We have to do the following steps: Add the following line to the webpack-config.js: Add the following line to the launch.json: Above I said it works "kind of", because I'm only able set breakpoints to some lines, e.g. in tsp-data-provider.ts, other lines in the same file are disabled. Any ideas? |
Thanks for the explanation. So it makes sense for these two components to each be published on npm. But probably not for
I think that's the best way forward, instead of inventing a twisted way top make all co-habitate. The extension can have its own simple repo and consume the two other components from npm. If we add it to the open-vsx build/publish service's config, a new |
I can do that, BTW, in the |
Ok, so I found the debugger under the "Sources" tab. I'm used to firefox where there is a Debugger tab. I can now officially debug!
Not sure why, but the file that we see in the debugger is some kind of pre-compiled version of the original typescript. I don't know where it got this. The lines where you put the breakpoints actually correspond to the original source code! I'll investigate how to have the proper files |
f41c54b
to
d28e13d
Compare
To build: first from the root run `yarn`. It will build all the packages for the vscode extension to work. After modifications to the code, the corresponding workspace needs to be built again. In packages/vscode-extension are 2 build commands that can also be watch: build/watch:extension to build the core vscode extension and build/watch:react to build the react app for the trace viewer. To run the extension from vscode itself, press F5 in vscode, or selecting the launch configuration called `Launch Extension`. The configuration does not automatically build anything. Signed-off-by: Quoc-Hao TRAN <quochao.tran.up@gmail.com> Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
But messages are not associate with a trace, so they remain visible even if we switch trace. They should be hidden when switching editor/webview. Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
TraceCompass is a trademark of Eclipse, so we can't use it here Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
This also allows to open experiments from the file explorer Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
** Not working ** The theia and electron apps do not compile correctly. You can add them to the main package.json file to try them out. SSL certificates are hard-coded paths in the app/browser directory. They can be modified by editing the package.json file, or proper keys can be symlinked to those paths: theiaCert.pem for the certificate and theiPrivKey.pem for the private key. Signed-off-by: Geneviève Bastien <gbastien+lttng@versatic.net>
Signed-off-by: Geneviève Bastien <gbastien@versatic.net>
* Rename the extension and add a publisher * Remove the theia-extension repo * Remove some console.log messages Signed-off-by: Geneviève Bastien <gbastien@versatic.net>
d28e13d
to
591375d
Compare
I updated and did some cleanup. I tried to build the vscode extension, but it would seem that the extension really should be at the root of the repo, as @marcdumais-work suggested, not as a workspace (vsce package does not find the modules that are in the root node_modules, just those in its own node_modules). What still needs to be done before merging and going forward 1- The vscode extension will move to its own new repo, as well as the 2 npm packages base and react-components, to their own repos as well. 2- Before doing so, we need to figure out under which github team/account (can't be theia-ide, or eclipse-theia, maybe tracecompass?), under which umbrella (ideally Eclipse Foundation to keep the Trace Compass naming, so we can publish as "tracecompass-community") and under what name (traceviewer??). It is being investigated. |
I advise not to stress too much about that part, or let you be blocked by these choices. If needed, go with clearly temporary names, that will need to be changed (I'm sure @MatthewKhouzam would have suggestions). It's not a big deal, and you can aim to find proper permanent names for v1.0, once all is clear and determined. |
I agree we/I don't want this PR to be blocked here for too long, we have to begin somewhere and start using it. So we could start a new repo under theia-ide (ironic) for the vscode-trace-extension that we won't publish yet on openvsx. But how about the 2 packages (base and react-components) that would need to go on npm for the trace-extension to use? Do we start publishing them under the same branding as the tsp-typescript-client and timeline-chart. What's your temporary thought on this? |
👍
Why not start publishing it now or as soon as we think it's useful, even if only for early testing? It will make it so much easier to consume that way - any Theia app that uses
I think these two components could wait before being published on
I think the current names for tsp-typescript-client and timeline-chart are fine temporary names and would not necessarily need to change after an eventual move the the Eclipse Foundation. If you can find similarly good names for the two new packages, it would work. We've had a very similar situation with the |
@bhufmann what's your opinion on @marcdumais-work 's last comment? Shall we start a new repo called vscode-trace-extension in theia-ide for now and keep the current one for the 2 packages? I'd update this PR to reflect that. |
Since the vscode extension will be moved to its own repo, only the split of the packages will remain in this repo. PR #172 replaces this one |
No description provided.