-
Notifications
You must be signed in to change notification settings - Fork 10
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
Download syntax file on build #22
Conversation
8f0877f
to
f73142f
Compare
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 the PR.
Aside from my in-line comments - I think the docs may need updating too?
https://github.com/hashicorp/vscode-terraform/blob/main/DEVELOPMENT.md
extensionVersion: string; | ||
syntaxVersion: string; | ||
preview: false; | ||
} |
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 we only end up using syntaxVersion
here - shall we drop the other two fields then?
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've used this same inteface across vscode-terraform, vscode-chl and vscode-sentinel.
It doesn't hurt to have it in there and we plan on moving to a framework when we know better what we need to build each of these extensions.
@@ -64,6 +67,8 @@ | |||
] | |||
}, | |||
"scripts": { | |||
"download:syntax": "ts-node ./build/download-syntax.ts", | |||
"preesbuild-base": "npm run download:syntax", |
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 not need to plumb this into the release process too anywhere in GHA, or is that basically what preesbuild-base
does here through the magic name? i.e. do we end up with downloaded grammar in /syntax
as part of npm ci
or npm run 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.
Npm script magic takes care of this
npm run package runs vsce
. vsce
looks for a script called vscode:prepublish
, which calls npm run esbuild-base
. NPM will run anything with pre
before the thing without the pre
, so preesbuild-base
runs before esbuild-base
. That finally downloads the file, and the rest of the chain can run.
If all this sounds complicated and obtuse, welcome to npm 😁 .
More seriously, this was the minimal code to get the job done. To make this better, we can use a build framework instead
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.
1f08899
to
0e36d0e
Compare
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 the PR, merge at will.
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.
LGTM!
Thank you for integrating got
!
package.json
Outdated
@@ -89,13 +94,16 @@ | |||
"@typescript-eslint/eslint-plugin": "^5.9.1", | |||
"@typescript-eslint/parser": "^5.9.1", | |||
"@vscode/test-electron": "^2.0.3", | |||
"axios": "^0.26.1", |
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.
axios
can be removed
15adc1a
to
7ab5c63
Compare
Downloads syntax file on build or package.