Skip to content
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: devtools #24

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

webfansplz
Copy link
Member

No description provided.

@netlify
Copy link

netlify bot commented Nov 22, 2022

Deploy Preview for vue-termui ready!

Name Link
🔨 Latest commit fb23644
🔍 Latest deploy log https://app.netlify.com/sites/vue-termui/deploys/637c504b916e4c0009189fab
😎 Deploy Preview https://deploy-preview-24--vue-termui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@codecov-commenter
Copy link

Codecov Report

Base: 64.64% // Head: 64.64% // No change to project coverage 👍

Coverage data is based on head (fb23644) compared to base (430d67b).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #24   +/-   ##
=======================================
  Coverage   64.64%   64.64%           
=======================================
  Files          44       44           
  Lines        4135     4135           
  Branches      195      195           
=======================================
  Hits         2673     2673           
  Misses       1462     1462           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

}
const app = express()
app.use('/__open-in-editor', launchMiddleware())
app.listen(SERVER_PORT)
Copy link
Member Author

@webfansplz webfansplz Nov 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@posva

I reused the @vue/devtools server instead of create new server.

I think reused the @vue/devtools server is enough, which that we can also focus all the logic on the devtools package. (I’ve tried reuse the vtui dev server,but it doesn’t work fine)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

If we move this server to the cli we can probably ensure it's always created before the app starts. It would also ensure only one express app is created even when restarting the app (force reload that isn't yet implemented)

Copy link
Collaborator

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think creating an extra package is a bit overkill here as we should be able to add it to the CLI and connect to the devtolls automatically without the user needing to add anything.

@@ -0,0 +1,44 @@
import devtools from '@vue/devtools'
import express from 'express'
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add one of these for later:

Suggested change
// @ts-ignore
// @ts-ignore: TODO: type the module

Comment on lines +1 to +5
if (process.env.NODE_ENV === 'development') {
import('@vue-termui/devtools').then(({ createDevtools }) => {
createDevtools().connect()
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird it needs to be first though 🤔 if it goes after the othor imports, it gives an error about the port being already used
I think there might be some other bugs pending in vue-devtools

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should probably find a way to add this directly from the vtui cli in development and skip it in production. That way the user doesn't need to write anything

title = 'vue-termui devtools',
} = options

// workaround for @vue/devtools
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be fixable in @vue/devtools as it has been done in the past like https://github.com/vuejs/devtools/pull/1780/files

}
const app = express()
app.use('/__open-in-editor', launchMiddleware())
app.listen(SERVER_PORT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah

If we move this server to the cli we can probably ensure it's always created before the app starts. It would also ensure only one express app is created even when restarting the app (force reload that isn't yet implemented)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants