Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Remove dependencies for downloading / unzipping #148

Merged
merged 5 commits into from
Mar 17, 2019
Merged

Remove dependencies for downloading / unzipping #148

merged 5 commits into from
Mar 17, 2019

Conversation

octref
Copy link
Contributor

@octref octref commented Mar 9, 2019

WIP. I plan to allow downloadAndUnzipVSCode to take a version too.

type Version = 'Insiders' | '1.32.0' | '1.31.1' | '1.31.0' ...

The script would download them to:

.vscode-test/
  /insiders
  /stable-1.32.0
  /stable-1.31.1
  /stable-1.31.0
  ...

@bpasero I'm wondering if we can also change the way we run test. Instead of saying users should run node ./node_modules/bin/test, maybe export a runTest function so it's easier to configure?

User's code could look like this:

import { runTests } from 'vscode-test';

async function go() {
  const vscodeExecutablePath = await downloadAndUnzipVSCode()

  const extPath = path.resolve(__dirname, '../../')
  const testPath = path.resolve(__dirname, '../test')
  const testWorkspace = path.resolve(__dirname, '../fixture')

  runTests(vscodeExecutablePath, extPath, testPath, testWorkspace)
}

@octref octref requested a review from bpasero March 9, 2019 06:01
@bpasero
Copy link
Member

bpasero commented Mar 10, 2019

@octref

@bpasero I'm wondering if we can also change the way we run test. Instead of saying users should run node ./node_modules/bin/test, maybe export a runTest function so it's easier to configure?

How would that work for extensions? Today we have the following command configuration in package.json:

"scripts": {
	"vscode:prepublish": "npm run compile",
	"compile": "tsc -p ./",
	"watch": "tsc -watch -p ./",
	"postinstall": "node ./node_modules/vscode/bin/install",
	"test": "npm run compile && node ./node_modules/vscode/bin/test"
}

How would the test command look like?

@bpasero
Copy link
Member

bpasero commented Mar 10, 2019

@octref btw I am not 100% sure removing the request library is wise. Are we certain we can handle things like proxy support properly without it?

@octref
Copy link
Contributor Author

octref commented Mar 10, 2019

@bpasero

Something like this: https://github.com/octref/vscode-test-ext/blob/master/package.json#L27

Also would make #111 and #124 possible, you can just install dependencies for fixture projects or 3rd party extensions yourself before running the tests.

Instead of implicit flags on CLI, we can now properly document and IntelliSense the options.

@octref
Copy link
Contributor Author

octref commented Mar 10, 2019

Are we certain we can handle things like proxy support properly without it?

I'll bring it back with https://github.com/TooTallNate/node-https-proxy-agent. request has many features we never use.

@bpasero
Copy link
Member

bpasero commented Mar 11, 2019

@octref we cannot really remove that test script though right? Because by now every extension has it in its package.json and we cannot update extensions to the newer solution?

@octref
Copy link
Contributor Author

octref commented Mar 11, 2019

@bpasero We could include the test script and still export the runTest function to users, so they can opt-in. But I think we should really move that to a dedicated module such as vscode-test.

If we do have #147 then we can start deprecating vscode module and add a warning to the test script, and maybe remove it sometime in the future.

@bpasero
Copy link
Member

bpasero commented Mar 11, 2019

@octref I think we can keep the test script in package.json but point into a script exposed from vscode-test NPM module without issues. That way we are not breaking or need any deprecation.

Basically, vscode module then depends on vscode-test module which has the test command (as well as a runText function if people want to use it directly).

Or was your idea to keep the two modules separate?

@octref
Copy link
Contributor Author

octref commented Mar 11, 2019

I think we can keep the test script in package.json but point into a script exposed from vscode-test NPM module without issues. That way we are not breaking or need any deprecation.

Sounds great. I can do that.

@octref octref marked this pull request as ready for review March 15, 2019 06:12
@octref
Copy link
Contributor Author

octref commented Mar 15, 2019

I think this is ready to review. I published https://github.com/microsoft/vscode-test and https://www.npmjs.com/package/vscode-test. You can see https://github.com/octref/vscode-test-sample for sample usage.

Things I added:

  • Simpler API. Now you can runTest without worrying about downloading & unzipping VS Code, but you do have the options to manually do that.
  • Proxy support.
  • Insider download support.

I didn't move the test runner yet. I can do that after this gets merged.

@bpasero
Copy link
Member

bpasero commented Mar 15, 2019

@octref great work, this makes the module much smaller. A few comments:

  • since you already started with converting test to use more modern ES6 features I suggest we do the same for install too

  • I am a little bit confused about the explicit listing of all versions of VSCode in lib/download.ts. We should not have a system where we need to update either vscode-test nor vscode module when doing a release
    image

  • you seem to be shipping the *.ts files with the module:

image

PS: should we switch to be using yarn for this module?

@octref
Copy link
Contributor Author

octref commented Mar 15, 2019

we do the same for install too

I can do that through another PR, but I thought we are moving away from postinstall scripts to directing people to install through @type/vscode: microsoft/vscode#70175.

listing of all versions of VSCode in lib/download.ts

Agreed. Removed it and is using https://update.code.visualstudio.com/api/releases/stable for validating version. microsoft/vscode-test@646c005

you seem to be shipping the *.ts files with the module:

Fixed.

should we switch to be using yarn for this module?

I think so. Me and Martin switched all satellite repos to using yarn.

@bpasero
Copy link
Member

bpasero commented Mar 17, 2019

I can do that through another PR, but I thought we are moving away from postinstall scripts to directing people to install through @type/vscode: microsoft/vscode#70175.

Right. The challenge I see here is that we will always need a way to point to the latest vscode.d.ts on master. So we would need to publish to @types/vscode almost as part of our nightly build or have a workaround for this so that we can still point to new API that is not released yet.

I think so. Me and Martin switched all satellite repos to using yarn.

Maybe that can be done as a separate PR.

Feel free to merge and publish under a new version 👍

@octref
Copy link
Contributor Author

octref commented Mar 17, 2019

the latest vscode.d.ts on master

I'll setup an Azure function that publishes our master/proposed API under NPM tag nightly and proposed.

Thanks for reviewing!

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

Successfully merging this pull request may close these issues.

2 participants