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

yarn upgrade #6255

Merged
merged 1 commit into from
Nov 12, 2019
Merged

yarn upgrade #6255

merged 1 commit into from
Nov 12, 2019

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Sep 24, 2019

What it does

Upgrade all our dependencies, according to our already specified ranges.

I had to tighten some constraints because the latest version of some
packages cause issues. Notably, the typescript compiler has an issue on
3.6.3 but not 3.5.3, so I constrained it such as it wouldn't ever match
the version that causes issues.

In the long run, we should be more careful with the version ranges we
use, especially with runtime dependencies.

Fixes #6500
DoNotFixes #6529

How to test

  • Build should pass.
  • Tests should pass.
  • Try anything and everything in the example applications to see if anything is broken.

Keep in mind that this commit only updates the lockfile, which means that we will use packages closer to what a fresh Theia build would pull, instead of the outdated versions pinned in the lockfile for the main repository.

Review checklist

Reminder for reviewers

@paul-marechal
Copy link
Member Author

paul-marechal commented Sep 24, 2019

Again, this shouldn't impact clients too much, unless I messed up with one of my source modifications.

Doing this made some things surface:

  • @theia/languages suffered from some circular dependencies, took me a while to figure out. Basically, exposing things via index.ts files is not too bad in my opinion, but we should never depend on them within the same extension. Index files are useful for dependents, but any internal import within a package should directly import from the most specific file.

  • Some of our typings are bogus and I had to resolve to using any because I couldn't find a proper fix for it. See UriAwareCommandHandler: we use a generic to accept only a URI or URI[] type. Issue is that it actually doesn't work. Instead of writing a long text trying to explain, you can remove the @ts-ignore I added and look at the error: something to do with potential incompatible subtypes. In practice it shouldn't happen, but with the way things are written now, it could slip.

  • TypeScript 3.6.3 doesn't like our MaybePromise: I opened an issue for it. In this case, tsc is only a dev-dep, so it is not too bad. I had to constrain the version a bit more so that we wouldn't pull the latest compiler, since we never know when things can break. It makes more sense to pin ourselves the compiler version, and change the range when we want to switch to a newer version.

  • Newer typings made a small bug surface: the html_element.style.overflow was set to null, but it actually does nothing (tried on firefox). We have to set some string, null doesn't do anything. I changed it, but I don't know if what I used is the intended value, please advise.

@paul-marechal
Copy link
Member Author

Ultimately I would like this yarn upgrade to be part of our release process. It would allow us to develop against the most up to date packages targeted by our ranges, and be closer to whatever would be pulled by newly built Theia applications (since our lockfile doesn't affect what dependents pull on install).

@paul-marechal paul-marechal mentioned this pull request Sep 24, 2019
1 task
@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Sep 25, 2019
@paul-marechal paul-marechal force-pushed the mp/yarn-upgrade branch 3 times, most recently from 1d1eb89 to 7a323cd Compare November 11, 2019 19:53
@paul-marechal paul-marechal mentioned this pull request Nov 12, 2019
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

code changes looks good to me

It would be good if someone double test.

Upgrade all our dependencies, according to our already specified ranges.

I had to tigthen some constraints because the latest version of some
packages cause issues. Notably, the typescript compiler has an issue on
3.6.3 but not 3.5.3, so I constrained it such as it wouldn't ever match
the version that causes issues.

In the long run, we should be more careful with the version ranges we
use, especially with runtime dependencies.

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

I performed a sanity check and everything works correctly :)
It is fine to merge once the CI is completed.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

Unfortunately it looks like Travis might be queued a while :(
I will approve nonetheless, but it'd be good to wait for CI to successfully pass before merging :)

@marcdumais-work
Copy link
Contributor

@marechal-p I think this type of dependencies overhaul should be accompanied by a sanity check that we are not pulling anything with a wrong license. Do you want to give it a try and let me know if you have questions?

https://github.com/eclipse-theia/theia/wiki/Registering-CQs#wip---new-ecd-theia-intellectual-property-clearance-approach-experimental

@paul-marechal
Copy link
Member Author

Checked the licenses, everything seems to be in order. Merging!

@akosyakov
Copy link
Member

I'm not sure about doing such upgrades without extensive testing. It breaks.

@paul-marechal
Copy link
Member Author

What did it break specifically?

@akosyakov
Copy link
Member

What did it break specifically?

debug console - #6551
content assist in json files apparently - #6542

I think upgrade is good, but we then should really test each extension.

@paul-marechal
Copy link
Member Author

I am not sure if this PR really broke the content assist, see #6542 (comment) it was already broken in new apps, but our repo was "uselessly" shielded from it apparently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

can't save file
5 participants