-
Notifications
You must be signed in to change notification settings - Fork 88
[DevTools] Dependencies and Basic Structure #2868
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
base: feature/dev-tools
Are you sure you want to change the base?
[DevTools] Dependencies and Basic Structure #2868
Conversation
🦋 Changeset detectedLatest commit: 4a46a58 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -28,3 +28,5 @@ allow-licenses: | |||
- X11 | |||
- zlib-acknowledgement | |||
- Zlib | |||
- LicenseRef-scancode-unknown-license-reference AND MIT |
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.
Create a github issue to later fix the issues before we can merge to main.
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.
Done. See: #2872 (comment)
@@ -0,0 +1,17 @@ | |||
#!/bin/bash |
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.
who's invoking this script?
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.
Accidental relic from old build process-- Deleting it.
async isDevToolsRunning(): Promise<boolean> { | ||
return this.isPortInUse(PortChecker.defaultDevtoolsPort); | ||
} |
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.
Ideally this shouldn't be in the PortChecker
class. We can either create a wrapper over this for devtools or just use isPortInUse
at places where we need to determine if devtools is running
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.
got it. going for the second option. My worry was that if we ever change the devtools port there isn't a clear place that constant should be, but at least for the forseeable future sandbox is the the only process using this, so I'm just defining it there.
# React + TypeScript + Vite | ||
|
||
This template provides a minimal setup to get React working in Vite with HMR and some ESLint rules. | ||
|
||
Currently, two official plugins are available: | ||
|
||
- [@vitejs/plugin-react](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react) uses [Babel](https://babeljs.io/) for Fast Refresh | ||
- [@vitejs/plugin-react-swc](https://github.com/vitejs/vite-plugin-react/blob/main/packages/plugin-react-swc) uses [SWC](https://swc.rs/) for Fast Refresh | ||
|
||
## Expanding the ESLint configuration |
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 file should be removed.
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.
Done.
@@ -0,0 +1,13 @@ | |||
<!doctype html> |
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.
There seems to be two sets of index.html and index.css. Do we need both?
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.
It looks like my placeholder files were referencing both, but they can easily be consolidated-- done.
printer.log( | ||
`DEBUG: Sandbox identifier: ${String(this.sandboxIdentifier)}, profile: ${String(this.profile)}`, | ||
LogLevel.DEBUG, | ||
); |
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 we need all these logger statements? Also we don't need to prepend DEBUG
in each statement, it should be done by the printer if required.
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.
They were useful in development, but I agree they can be deleted now. Cutting a bunch of the excess debug logging.
"update:api": "api-extractor run --local" | ||
"update:api": "api-extractor run --local", | ||
"clean": "rm -rf lib && rm -rf src/commands/sandbox/sandbox-devtools/public", | ||
"build:devtools": "cd src/commands/sandbox/sandbox-devtools/react-app && npm install && npm run build", |
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 you cd
, don't you also need to come back to the cwd
?
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.
in npm scripts, each command runs in its own shell subprocess, so when the command finishes, that subprocess exits and youre back in the cwd
"build:devtools": "cd src/commands/sandbox/sandbox-devtools/react-app && npm install && npm run build", | ||
"build:ts": "tsc --build --force", | ||
"copy:devtools": "mkdir -p lib/commands/sandbox/sandbox-devtools/react-app/dist && cp -r src/commands/sandbox/sandbox-devtools/react-app/dist/* lib/commands/sandbox/sandbox-devtools/react-app/dist/ || echo 'No React app build found'", | ||
"build": "npm run clean && npm run build:devtools && npm run build:ts && npm run copy:devtools && chmod +x lib/ampx.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.
why clean every time we build?
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.
Earlier in development build was giving me lots of trouble with changing app assets and file structure, and cleaning every time was the best way to ensure old assets weren't affecting my app. Now, builds are much less frequent, so we can probably take out the clean, but maybe have another script for build:clean that does run clean and run build?
packages/cli/package.json
Outdated
"build:ts": "tsc --build --force", | ||
"copy:devtools": "mkdir -p lib/commands/sandbox/sandbox-devtools/react-app/dist && cp -r src/commands/sandbox/sandbox-devtools/react-app/dist/* lib/commands/sandbox/sandbox-devtools/react-app/dist/ || echo 'No React app build found'", | ||
"build": "npm run clean && npm run build:devtools && npm run build:ts && npm run copy:devtools && chmod +x lib/ampx.js", | ||
"postinstall": "npm run build:devtools && npm run copy:devtools" |
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.
who's calling/running this command?
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.
For build, mostly developers (me). The app gets rebuilt often to show changes. Postinstall is no longer useful, removing it.
resourceConfigChanged: [ | ||
async () => { | ||
try { | ||
// Notify that resource configuration has changed | ||
printer.log( | ||
'[Sandbox] Resource configuration changed, updating resources...', | ||
LogLevel.DEBUG, | ||
); | ||
} catch (error) { | ||
printer.print( | ||
`${format.error( | ||
'Failed to handle resource configuration change.', | ||
)} ${format.error(error)}`, | ||
); | ||
} | ||
}, | ||
], |
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.
who's emitting this event? Should this be in PR2?
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.
yep, removing this and the corresponding field in the type def
Includes dependencies, very elementary facade of react app, and basic devtools command functionality which just prints a statement instead of opening the app.