Skip to content

build: bump compatibility versions of the iOS SDK and Node.js #13948

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

Closed
wants to merge 3 commits into from
Closed

build: bump compatibility versions of the iOS SDK and Node.js #13948

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d73831c
chore(ios): bump Xcode and iOS compatibility versions
hansemannn Nov 16, 2023
f55b32e
chore(cli): bump Node.js compatibility versions
hansemannn Nov 16, 2023
1819b6a
chore: add Node.js 20 support
hansemannn Nov 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions iphone/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
"minIosVersion": "13.0",
"minWatchosVersion": "6.0",
"vendorDependencies": {
"xcode": ">=12.0 <=14.x",
"ios sdk": ">=13.0 <=16.x"
"xcode": ">=12.0 <=15.x",
"ios sdk": ">=13.0 <=17.x"
},
"engines": {
"node": ">=12.13.0"
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@
"url": "git://github.com/tidev/titanium_mobile.git"
},
"vendorDependencies": {
"node": "12.x || 14.x || 16.x"
"node": "12.x || 14.x || 16.x || 18.x || 20.x"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@cb1kenobi I wonder if we should remove everything pre-16, but in a SDK 13.0.0

Copy link
Contributor

Choose a reason for hiding this comment

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

#13807

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's what I propose:

  1. Don't touch anything in CLI 6.x or SDK 12.x
  2. In CLI v7 (Titanium CLI v7 Beta 1 titanium-cli#618), min Node version set to 18 (done)
  3. In CLI v7, rip out the vendorDependencies check (todo)
  4. In SDK 13, do nothing
  5. In SDK 14, do nothing
  6. In SDK 15, rip out vendor dependencies

Why rip out vendor dependencies?

  1. It's a pain to maintain
  2. "engines" didn't exist back in the day, but now it's a tool we can use to keep users moving forward
  3. Node support dictated by actively supported SDK releases, so regular release cadence will drop support for older Node.js versions naturally
  4. Rarely do we have Node compatibility issues with SDK dependencies

Note that we cannot remove vendorDependencies from the SDK's package.json until every user has updated to CLI v7. CLI v6 explicitly checks for that property with no guard. :(

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Please, please, please do not rip out anything or break things for now. As nice as the CLI v7 is, it's far away from a stable / tested release and will require some major upgrades to run stable. Fixing the warning in SDK 12.x should be the most convenient way for most users to continue working without updating their Node.js setup, while developers that have the latest LTS tooling can continue working as well, but without warnings.

Copy link
Contributor

@m1ga m1ga Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

+1 for removing it (vendorDependencies)!
But I think we should add up to 20 in the package.json for 12.x just to get rid of the warning as there are no issues with using it - keeping the minimum as it is for now.
This makes it easier for new users to just grab the latest node version from the page and not have a warning right away

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, yes, add 18 and 20 to SDK 12 to suppress the warning.

Definitely not suggesting we break anything anytime soon. Once v7 is merged and published as latest (it'll be a while), we can circle back and do a final v6 release that adds a guard around vendorDependencies and a message to update to v7. I want to avoid touching the CLI until v6 is in a maintenance branch and v7 is merged into master. The merge conflicts are going to be impossible to fix.

},
"engines": {
"node": ">=12.13.0"
Expand Down