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

[#1872] Update Node version to 18 #2081

Merged
merged 14 commits into from
Mar 19, 2024

Conversation

sopa301
Copy link
Contributor

@sopa301 sopa301 commented Jan 16, 2024

Fixes #1872

Proposed commit message

Node.js 16 has reached its end-of-life on 11 September 2023.

Let us update Node.js to use Node 18 in the GitHub workflows.

Other information

The update involved removing the depreciated node-sass.

@sopa301 sopa301 changed the title Update Node version to 18 [#1872] Update Node version to 18 Jan 16, 2024
@sopa301 sopa301 marked this pull request as ready for review January 16, 2024 08:59
@sopa301 sopa301 marked this pull request as draft January 16, 2024 09:02
@sopa301
Copy link
Contributor Author

sopa301 commented Jan 18, 2024

Closed because I don't have the required competencies (as of now) to properly fix the issue. Here are some things I learned in the process.

  • Updating Node to 18 requires node-sass to be updated to at least 8.0
  • However, node-sass has been depreciated in favor of dart-sass (sass), which is already used in the project.
  • Additionally, node-sass on windows has a few more requirements that make it harder to set up.
  • Therefore, it would be easier to remove node-sass and use sass exclusively.
  • Attempting to do so resulted in a bug similar to GitHub Actions: Cypress tests are failing #1784

@sopa301 sopa301 closed this Jan 18, 2024
Copy link
Contributor

The following links are for previewing this pull request:

@ckcherry23
Copy link
Member

Thanks @sopa301 for your detailed investigation regarding the issue and reporting it here!
We can figure out a fix for this in a future PR.

@sopa301 sopa301 reopened this Feb 18, 2024
docs/dg/settingUp.md Outdated Show resolved Hide resolved
@ckcherry23 ckcherry23 requested a review from a team February 19, 2024 07:17
@ckcherry23 ckcherry23 marked this pull request as ready for review February 19, 2024 07:18
Co-authored-by: Charisma Kausar <[email protected]>
@sopa301
Copy link
Contributor Author

sopa301 commented Feb 19, 2024

It's interesting to note #2102 somehow fixed the bug mentioned above.

Copy link
Member

@ckcherry23 ckcherry23 left a comment

Choose a reason for hiding this comment

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

LGTM!

@ckcherry23 ckcherry23 requested a review from a team February 19, 2024 09:09
@ckcherry23 ckcherry23 requested a review from vvidday March 14, 2024 04:23
Copy link
Contributor

@vvidday vvidday left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this @sopa301, can you look into resolving the merge conflict?

@sopa301
Copy link
Contributor Author

sopa301 commented Mar 16, 2024

@vvidday Updated!

@vvidday
Copy link
Contributor

vvidday commented Mar 16, 2024

@vvidday Updated!

thanks! :)

@ckcherry23 ckcherry23 merged commit bb977ea into reposense:master Mar 19, 2024
10 checks passed
Copy link
Contributor

The following links are for previewing this pull request:

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

Successfully merging this pull request may close these issues.

Frontend DevOps: Upgrade Node to 16 then 18
3 participants