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

Re-add code to download loc files in gulpfile.js for VS build #1622

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

EricCornelson
Copy link
Contributor

No description provided.

gulpfile.js Outdated
@@ -17,6 +17,10 @@ const util = require('util');
const deepmerge = require('deepmerge');
const esbuild = require('esbuild');
const esbuildPlugins = require('./src/build/esbuildPlugins');
const signale = require('signale');
const got = require('got').default;
const unzipper = require('unzipper');
Copy link
Member

@connor4312 connor4312 Mar 29, 2023

Choose a reason for hiding this comment

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

It looks like this is missing in the package.jons.

Also, I would prefer using jszip over unzipper; in @vscode/test-electron we recently moved to jszip due to issues and CG alerts on the seemingly-abandoned unzipper package.

Usage of jszip in vscode-test: https://github.com/microsoft/vscode-test/blob/83f719c7364c316ae1736e75ef1a505ad88461f1/lib/download.ts#L278-L294

gulpfile.js Outdated
Comment on lines 34 to 35
const distDir = 'dist';
const distSrcDir = `${distDir}/src`;
Copy link
Member

Choose a reason for hiding this comment

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

These are defined as buildDir and buildSrcDir already

gulpfile.js Outdated

const buffer = new streamBuffers.WritableStreamBuffer();
const locale = match[1];
entry.pipe(buffer).on('finish', () => {
Copy link
Member

Choose a reason for hiding this comment

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

You can use the pipeline method of streams, as in the code example I linked above, instead of manual pipes 🙂

connor4312
connor4312 previously approved these changes Mar 30, 2023
@connor4312
Copy link
Member

Thanks! Code changes look good, but it looks like you npm install'd with the devdiv packages registry, instead of public npm, which causes the build to fail. You should be able to fix this by resetting the change to package-lock.json (git checkout main package-lock.json), commenting out any registry info in your ~/.npmrc, and npm-installing again.

@connor4312 connor4312 merged commit 746e45e into microsoft:main Mar 30, 2023
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