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

fix: make install-local script work, update react to 17 VSCODE-310 #471

Merged
merged 8 commits into from
Feb 27, 2023

Conversation

Anemy
Copy link
Member

@Anemy Anemy commented Feb 3, 2023

VSCODE-310

  • Updates react to 17 for leafygreen.
  • Re-adds a workaround patch we use in our CI package script that we had removed from our local install packaging script.

Note

Before we merge this we'll need to update our mongodb-data-service dependency to pull in a new version of @mongodb-js/compass-utils so that the @electron/remote dependency isn't required. PR: mongodb-js/compass#4032. That will make the npm list command that vsce package runs stop failing so install-local starts working again. I manually tested that things work after that fix by doing some node_modules updates and package-lock changes locally. Opening as a draft for now.

Addresses #353

package.json Outdated Show resolved Hide resolved
@Anemy
Copy link
Member Author

Anemy commented Feb 7, 2023

Updating our mongodb-data-service dependency took a good amount of changes as it updated the api to use promises instead of callbacks. Added electron to dev deps to make the install script work as it appears npm list --production --parseable --depth=99999 --loglevel=error will fail on optional dependencies. Not sure if there's a way to work around that without including more monkey patches like the no-npm-list-fail.js was doing.

@Anemy
Copy link
Member Author

Anemy commented Feb 7, 2023

Error: Command failed: npm list --production --parseable --depth=99999 --loglevel=error
npm ERR! code ELSPROBLEMS
npm ERR! invalid: mongodb@4.13.0 D:\a\vscode\vscode\node_modules\mongodb
npm ERR! invalid: js-yaml@3.14.0 D:\a\vscode\vscode\node_modules\js-yaml

Not sure why these are showing up, looking into.

@Anemy
Copy link
Member Author

Anemy commented Feb 15, 2023

Opening this up for review. I tried to remove the workaround the no-npm-list-fail.js was doing but wasn't able to for the ci. I think this is something to do with the install omitting optional dependencies:

run: npm ci --omit=optional

When it runs that install the npm package list command run by vsce fails:
npm list --production --parseable --depth=99999 --loglevel=error
I don't know the exact reasoning why, I think it's something to do with transitive optional peer dependencies being hoisted differently. For now I'll keep things as. It does mean that the local-install script is susceptible to breaking again without the CI reporting it. Once we can remove the node workaround that silences npm list errors we'll have more confidence that the local install script works. I created a ticket to track that VSCODE-370. Regardless, folks should be able to run npm run local-install again and have it install the extension to their VSCode instance.

@Anemy Anemy marked this pull request as ready for review February 15, 2023 21:14
@Anemy Anemy requested a review from alenakhineika February 16, 2023 15:56
import { beforeEach, afterEach } from 'mocha';
import { connect } from 'mongodb-data-service';
import { before, after, beforeEach, afterEach } from 'mocha';
import { connect, DataServiceImpl } from 'mongodb-data-service';
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting. My branch suddenly started failing on this test suite. I copied this refactored version to it and now tests pass. Hope you don't mind I copied it here: 8d77ef3#diff-a468a59574d19865f8eea5715e9ceaa187fb55debcddb46eee85c55236408c6d

Copy link
Member Author

Choose a reason for hiding this comment

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

I just pushed up a new commit that removes usage of DataServiceImpl instead using only the DataService type. I think you'll want to update it again - sorry!

Copy link
Contributor

Choose a reason for hiding this comment

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

It actually started to fail again with the old version I copied. Something weird is happening with this test suit.

Screenshot 2023-02-27 at 18 09 33

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what's happening at a glance. Are the dependencies possible from a different branch? Maybe try and 'npm install'

@Anemy Anemy merged commit 946d790 into main Feb 27, 2023
@Anemy Anemy deleted the fix-local-install branch February 27, 2023 19:53
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.

2 participants