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.

Sign up for GitHub

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

Jump to bottom

upgrade nodejs to 18.17.0 #15899

Merged
merged 9 commits into from
Aug 28, 2023
Merged

upgrade nodejs to 18.17.0 #15899

merged 9 commits into from
Aug 28, 2023

Conversation

PPpro
Copy link
Contributor

@PPpro PPpro commented Aug 9, 2023 •
edited
Loading

Re: #15895

Changelog

  • upgrade nodejs to 18.17.0

deps PR

Continuous Integration

This pull request:

  • needs automatic test cases check.

    Manual trigger with @cocos-robot run test cases afterward.

  • does not change any runtime related code or build configuration

    If any reviewer thinks the CI checks are needed, please uncheck this option, then close and reopen the issue.

Compatibility Check

This pull request:

  • changes public API, and have ensured backward compatibility with deprecated features.
  • affects platform compatibility, e.g. system version, browser version, platform sdk version, platform toolchain, language version, hardware compatibility etc.
  • affects file structure of the build package or build configuration which requires user project upgrade.
  • introduces breaking changes, please list all changes, affected features and the scope of violation.

@PPpro PPpro marked this pull request as draft August 9, 2023 02:55
@PPpro PPpro force-pushed the 381-nvm branch 2 times, most recently from 53468f4 to 584c32d Compare August 9, 2023 09:42
@PPpro
Copy link
Contributor Author

PPpro commented Aug 9, 2023 •
edited
Loading

don't merge
wait for @VisualSJ to confirm which branch the upgrading will be merged to, 3.8 or 3.9

@minggo
Copy link
Contributor

minggo commented Aug 9, 2023

Will it break compatibility?

@PPpro
Copy link
Contributor Author

PPpro commented Aug 10, 2023 •
edited
Loading

no, but we need to merged with editor upgrade, that maight break compatibility

shrinktofit
shrinktofit reviewed Aug 11, 2023
View reviewed changes
package.json Outdated
@@ -10,6 +10,7 @@
"module": "index.js",
"scripts": {
"server": "http-server . -p 8002 -o playground/index.html",
"preinstall": "node ./scripts/detect-npm-version.js",
Copy link
Contributor

@shrinktofit shrinktofit Aug 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Consider using https://docs.npmjs.com/cli/v9/configuring-npm/package-json#engines and https://docs.npmjs.com/cli/v9/using-npm/config#engine-strict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@PPpro PPpro requested a review from VisualSJ August 14, 2023 01:53
@PPpro PPpro marked this pull request as ready for review August 15, 2023 06:09
minggo
minggo reviewed Aug 15, 2023
View reviewed changes
cocos/core/data/decorators/property.ts Show resolved Hide resolved
pal/input/web/accelerometer-input.ts
if (window.DeviceMotionEvent && typeof DeviceMotionEvent.requestPermission === 'function') {
DeviceMotionEvent.requestPermission().then((response) => {
// NOTE: since TS 4.4, requestPermission is not defined in class DeviceMotionEvent, this should be a breaking change in TS.
if (window.DeviceMotionEvent && typeof (DeviceMotionEvent as any).requestPermission === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the comment mean? As it just convert it to any.

Copy link
Contributor Author

@PPpro PPpro Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

that means there is a breaking change between TS 4.3 and TS 4.4

DeviceMotionEvent is defined differently in both version, so we can only convert DeviceMotionEvent as any to access the requestPermission property

Copy link
Contributor

Choose a reason for hiding this comment

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

If it is not defined in DeviceMotionEvent , just make it as any can fix the problem?

Copy link
Contributor Author

@PPpro PPpro Aug 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

yes, requestPermission is a safari-specific method of DeviceMotionEvent, not a web standard method, so it's not defined in TS standard dom lib since 4.4 in lib.dom.d.ts

but as a cross-platform engine, we still need to keep this broswer compability

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see https://caniuse.com/mdn-api_devicemotionevent_requestpermission_static

Copy link
Contributor

Choose a reason for hiding this comment

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

What i confused is the comment. Why need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the comment, maybe this would help understanding
99cb40d

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep. It is more clear. But how do you know it is a bug of lib.dom.d.ts or it is already removed in the specification, and some web platforms may remove the function.

Copy link
Contributor Author

@PPpro PPpro Aug 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

here is the release note of TS 4.4, the breaking change list here https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-4.html#libdts-changes-for-typescript-44
it should be a breaking change, not a bug

some web platforms may remove the function

the declaration of requestPermission is only removed in TS 4.4 lib.dom.d.ts .
and some web platforms actually didn't remove it, they just never support this interface, so our implementation here need to check whether requestPermission is defined before we use it

tests/core/serialization/deserialize-capacity.test.ts Show resolved Hide resolved
@PPpro PPpro marked this pull request as draft August 16, 2023 03:42
@PPpro PPpro changed the title upgrade nodejs to 18.17.0 [don't merge] upgrade nodejs to 18.17.0 Aug 16, 2023
@PPpro
upgrade nodejs to 18.17.0
36bae4e 
fix compile-native-ts

fix type

fix eslint

fix node version on native-binding

test native ci

fix eslint

fix jest version

upgrade ts-jest

fix

reduce EXPECTED_CAPACITY

update ccbuidl

Revert "test native ci"

This reverts commit 795c8c6.

# Conflicts:
#	scripts/native-pack-tool/package-lock.json
@PPpro
detect node version
8f83a2f 
# Conflicts:
#	package.json
@PPpro
use engine-strict option
04d9e64
@PPpro
Update .nvmrc
e238343
@PPpro
Update pal/input/web/accelerometer-input.ts
e3dba35
@PPpro
fix conflict
5db1654
@PPpro PPpro changed the base branch from v3.8.1 to v3.8.2 August 24, 2023 09:36
@PPpro PPpro changed the title [don't merge] upgrade nodejs to 18.17.0 upgrade nodejs to 18.17.0 Aug 24, 2023
@PPpro PPpro marked this pull request as ready for review August 24, 2023 09:36
@PPpro PPpro closed this Aug 24, 2023
@PPpro PPpro reopened this Aug 24, 2023
@PPpro PPpro mentioned this pull request Aug 25, 2023
Merged
@VisualSJ VisualSJ merged commit 9daf2c6 into cocos:v3.8.2 Aug 28, 2023
9 of 10 checks passed
@PPpro PPpro deleted the 381-nvm branch August 28, 2023 07:53
@github-actions
Copy link

@PPpro ❗ There was an error during the execution of the tasks. Please check the logs for more details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shrinktofit shrinktofit shrinktofit approved these changes

@VisualSJ VisualSJ VisualSJ approved these changes

@minggo minggo minggo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@PPpro @minggo @VisualSJ @shrinktofit