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

feat(node-18-upgrade): Upgrade to node 18 #702

Merged
merged 5 commits into from
Aug 14, 2023
Merged

Conversation

JChan106
Copy link
Contributor

This is to upgrade box-annotations to use node 18. This requires replacing node-sass with sass, similar to the upgrades in box-content-preview and BUIE. We did not split this into a separate PR because that upgrade requires node 14+, so instead of upgrading to node 14 in another PR and then node 18 in this PR, we decided to upgrade them at once.

This upgrade to node 18 is similar to the upgrade for box-content-preview. We use the same workaround BUIE and box-content-preview use for the OpenSSL issue.

@JChan106 JChan106 requested a review from a team as a code owner July 27, 2023 22:56
package.json Outdated
@@ -55,7 +55,7 @@
"babel-plugin-react-intl": "^7.5.1",
"babel-plugin-transform-require-ignore": "^0.1.1",
"box-node-sdk": "^1.33.0",
"box-ui-elements": "^15.0.0-beta.6",
"box-ui-elements": "18.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ^ removed on purpose? Do we need this exact version of BUIE?

Copy link
Contributor Author

@JChan106 JChan106 Jul 27, 2023

Choose a reason for hiding this comment

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

Hm good point, I referenced how box-content-preview used BUIE, and it seems like it didn't use ^, so I followed the same principle. I don't see an issue with just freezing the major version, since it is a dependency we trust. Updated

package.json Outdated
@@ -154,5 +153,8 @@
},
"resolutions": {
"uglify-es": "3.3.4"
},
"dependencies": {
"sass": "^1.64.1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we move this to devDependencies? Otherwise, it will be included as a transitive dependency by its consumers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah my mistake, will do

Copy link
Collaborator

@jstoffan jstoffan left a comment

Choose a reason for hiding this comment

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

Awesome!

@mergify mergify bot merged commit 34b49ab into box:master Aug 14, 2023
@JChan106 JChan106 deleted the node-18 branch August 14, 2023 16:10
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.

3 participants