-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Creates a smaller build of TypeScript which just has the language services #33584
Conversation
@typescript-bot pack this |
…a separate build step for creating and publishing vs configuring the build
@typescript-bot pack this |
I can't verify this works 100% on the TypeScript account, it all hooked up right on mine - so sorry if it means a bit of extra work for whoever is deploying. |
Not blocking for this, but I'll fix in a next PR : using |
exec(`node scripts/configureLanguageServiceBuild.js ${join(packagePath, "package.json")}`); | ||
|
||
step(`Deploying the language service`); | ||
exec("npm publish --access public", { cwd: packagePath }); |
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 have any other scripts that auto-publish? I don’t know what our publish process is like, but I assume it runs in CI. I would lean toward adding automation there rather than scripted into postpublish
, as principle of least surprise.
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.
IOW, the code seems fine (but I’ve never looked at our packing process before either, so I’m not the best one to review), but I’m skeptical of the trigger
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.
Nope, it doesn't happen on CI (I am happy to move it to a new step in there) and/or look at moving to deploy on CI ( e.g microsoft/types-publisher#659 ) and all we do is ship a tag to indicate we want a release
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.
Oh really? I figured we publish from here: https://dev.azure.com/typescript/TypeScript/_release?definitionId=1&view=mine&_a=releases
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.
We do! It definitely happens on CI! Anyone caught not publishing on CI will be [redacted]!
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.
Specifically, we publish a prepared tarball directly, using npm publish $(System.ArtifactsDirectory)/typescript.tgz --tag $(PUBLISH_TAG)
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.
So a postpublish
script is actually wholly nonfunctional, since we don't even publish in a clone of our repo 😄
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.
Hah! OK cool, perfect!
I'll that and add the extra step on CI 👯♂
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.
Cool - updated the deployment docs, and removed the auto-publish. Will give this a few hours for last feedback and merge today. Then once that's in I'll add the step to the pipeline.
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 script is not executable in the release pipeline, as far as I'm aware. Releases are not executed within the context of the repo. (There's good reason for this, too, it means at no point does potentially malicious code get a chance to execute in a context where we have our publishing key set!) The code in the repo does not exist in the release pipeline. To publish a second package, you would need to publish a second pipeline artifact with the secondary tarball (which is just done with a few commands in the build pipeline, but I imagine you'll need to add a call to the configure
script in addition to replicating those), and publish in a similar way as we do the first (with the npm publish
release pipeline task).
👍 it's unlikely the beta will have this package, but it's good to go and for me to look at the pipelining next. |
Relates to #27891
Creates a sub-set build of TypeScript which is only the
require("typescript")
api. This means that folks who build tools which use TypeScript under the hood can rely on this smaller dependency.TODO:
Get the current package and extract it to a subfolder, which the script can work on
Make the deploy process also ship the additional package
Add additional automated tests of the dependency
Test this build on a few repos
I've tested moving some medium projects like danger-js and tsd to use this module in their imports and it's worked, but the projects need to be up to date as it's the 3.7.0 build.