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

Updated dependencies #316

Merged
merged 1 commit into from
Nov 18, 2024
Merged

Updated dependencies #316

merged 1 commit into from
Nov 18, 2024

Conversation

vladislavarsenev
Copy link

@vladislavarsenev vladislavarsenev commented Nov 14, 2024

Updated following dependencies:

  • prettier to 3rd ver
  • typescript to 5th ver

Updated tools:

  • minimum Node.js version is 18.x
  • minimum Yarn version is 4.5.1

Copy link
Collaborator

@ayusharma ayusharma left a comment

Choose a reason for hiding this comment

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

Thanks @vladislavarsenev. Left couple of small suggestions, other than that everything looks good to me 🚀 Thank you ❤️ I would approve after the discussion on open questions in PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this file needed ?

Copy link
Author

Choose a reason for hiding this comment

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

Nice catch, thank you!

image

package.json Outdated
"engines": {
"node": ">18.12"
},
"packageManager": "[email protected]",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The packageManager field is in experimental. I would recommend to not to add. https://nodejs.org/docs/latest-v20.x/api/all.html#all_packages_packagemanager

Copy link
Author

Choose a reason for hiding this comment

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

I've deleted "packageManager" from package.json and removed corepack from CI since packageManager was mandatory thing for using it. However, I would like to notice the fact that although all these features are experimental, they have been around for 4 years.

I should ask you about --experimental-vm-modules which I added for launching tests with the newest prettier version and it was necessary since prettier@3 utilizes dynamic import. I tried quickly transpile code to omit --experimental-vm-modules, but failed. Should I to figure out how to do so without --experimental-vm-modules?

@@ -6,6 +6,6 @@ import { getAllCommentsFromNodes } from './get-all-comments-from-nodes';
export const isSortImportsIgnored = (nodes: Statement[]) =>
getAllCommentsFromNodes(nodes).some(
(comment) =>
comment.loc.start.line === 1 &&
comment.loc?.start.line === 1 &&
Copy link
Collaborator

@ayusharma ayusharma Nov 18, 2024

Choose a reason for hiding this comment

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

May be 😉

Suggested change
comment.loc?.start.line === 1 &&
comment.loc && comment.loc.start.line === 1 &&

Copy link
Author

Choose a reason for hiding this comment

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

Fixed, but what a benefit to write in this way? It would never equal 1 if comment.loc was undefined.

Copy link
Collaborator

@byara byara left a comment

Choose a reason for hiding this comment

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

I left one comment but it is not a blocker. Thank you so much for taking a look 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just a suggestion (which I think may unblock other things too) is to maybe unpin most of these dependencies?
What do you think @ayusharma?
Because right now we are using exact versions of certain dependencies. I was wondering if we could move away from this.

Copy link
Author

Choose a reason for hiding this comment

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

Do you want to use "compatible with version" or "approximately equivalent" modes? What are benefits of these approaches? I'm curious because last 4 or 5 years we used exact versions on all projects and it was more predictable and convenient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think ^ (Compatible with version) is something we want to go for 👍

Copy link
Author

Choose a reason for hiding this comment

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

Do you know about any limitations of using ^ right now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think there were some issues when people had different version of the same dependencies on their project? 🤔 Or am I missing something @ayusharma

@ayusharma ayusharma marked this pull request as ready for review November 18, 2024 18:35
@ayusharma ayusharma merged commit 1a1f130 into trivago:v5 Nov 18, 2024
3 checks passed
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