Move vscode builtin production from yarn to npm #132
Conversation
|
@rschnekenbu is there documentation that needs to be updated with this change? |
|
@tsmaeder, probably some README or how-to-build documentation to update, indeed. I will update this PR |
rschnekenbu
force-pushed
the
issues/131-migrate-to-npm
branch
from
ec31c74
to
7455a52
Compare
|
@tsmaeder, I updated the documentation, some commands now relying on npm rather than yarn on vscode repo. I took also the opportunity to update the vscode commit ref to tag 1.94.2 (current latest release on 1.94 stream). I also updated some part of the documentation that had some markdown formatting issues or warnings. |
| ## Building | ||
| Building the exensions from VS Code is done simply with | ||
|
|
||
| Building the exensions from VS Code is done simply from this repository root folder with | ||
|
|
||
| yarn build:extensions |
There was a problem hiding this comment.
no, the vscode-builtin-extension project itself still relies on yarn. Only vscode submodule was migrated to npm. I was waiting for the success on theia project migration to check if it should move also to npm or any other choice. By the time of the PR, this was not yet finalized.
I should have mentionned it in the review text. BTW, should we use the default PR template for this project also? The follow up section would make sense here, as the project may be also upgrade once npm migration is finalized on main theia repo.
| @@ -39,15 +44,13 @@ Once we have built our extensions, we can packge them into `*.vsix`-files using | |||
| The script will produce `*.vsix` files in a folder called `./dist`. The vsix files will be named like `<name>-<vscode-version>.vsix`. Note that the publisher (msvscode) | |||
| is not included. | |||
|
|
|||
| If you want to create a prerelease version, you can do so by invoking | |||
| If you want to create a prerelease version, you can do so by invoking | |||
|
|
|||
| We now have to perform the IP checks for the "external builtins". These are extensions which are not developed as part of the VS code repository, but which are still included as part of the | ||
| VS Code product. They are described in the `product.json` file which lives at the root of the VS Code repository. There is a package script which will clone the relevant repos and check out | ||
| the correct tag into a folder named `external-builtins`. | ||
|
|
||
| yarn get-external-builtin |
| to first run the [dash-licenses](https://github.com/eclipse/dash-licenses) tool to check the dependencies of the bulit-in | ||
|
|
||
| To prepare for the IP checks, you'll have to perform the setup steps from [Building.md](./Building.md#setup). Now we need | ||
| to first run the [dash-licenses](https://github.com/eclipse/dash-licenses) tool to check the dependencies of the built-in | ||
| extensions for compatibility with the Theia license. There are a couple of package scripts helping with this: the following sequence downloads the dash-licenses jar to the current directory and then runs the `dash-licenses` for all relevant extensions in the `vscode/extensions` directory. | ||
|
|
||
| yarn download:dash-licenses |
| * @param {string} sourceDir - The path of the source folder. | ||
| * @param {string} targetDir - The path of the destination folder. | ||
| */ | ||
| function copyYarnLock(sourceDir, targetDir) { |
There was a problem hiding this comment.
Do we not need a package-lock.json instead?
There was a problem hiding this comment.
As far as I could see, vsce tool does not need any package-lock.json. It was used as far as I can see to detect the kind of tool used, but it is ignored in the produced vsix.
I did a side-by-side compare of the 2 generated vsix extensions between 1.88.1 and 1.94.2. I do not see any differences between the 2 with the current configuration, especially comparing typescript-language-feature and emmet, that have some specific handling during build and packaging process.
|
@tsmaeder, as mentionned in my comments I have just added, this repo still relies on yarn. Only vscode submodule and related scripts have been migrated to npm. |
There was a problem hiding this comment.
The changes look good to me. I just ran through everything, and after a minor modification (see below) everything works as expected.
For some reason, npm install crashes with an error when trying to install the dependencies of vscode. I needed to add "name": "vscode-builtin-extensions" to our own package.json file in order to make it work. Please add that name property (which is good practice anyway) to our root package.json.
|
@tsmaeder In my opinion this is good to go. I don't think we necessarily need to migrate this repo fully to npm. What do you think? |
|
@rschnekenbu Do you want to squash your changes now? It seems like only "rebase and merge" is enabled for this repo, so I cannot squash the changes for you. |
Also updates builtin to 1.94.2 fixes eclipse-theia#131 contributed on behalf of STMicroelectronics Signed-off-by: Remi Schnekenburger <[email protected]>
rschnekenbu
force-pushed
the
issues/131-migrate-to-npm
branch
from
67ef180
to
6b561ae
Compare
|
@msujew, do you want to have a last look or are we good to go? |
|
@rschnekenbu Feel free to merge :) |
Migrate the package manager of VS Code from yarn to npm
Also updates to vscode subrepo to 1.94.2, as this is the first version to rely on npm.
fixes #131
contributed on behalf of STMicroelectronics