fix deprecation warnings in node 24#109
Conversation
…ort via cmd /c Co-authored-by: ptandler <934595+ptandler@users.noreply.github.com>
Co-authored-by: ptandler <934595+ptandler@users.noreply.github.com>
|
Have you tested these changes with previous Node.js versions? I'd like to confirm if they are backward compatible with older releases. |
|
Hi! @ptandler I’ve created a new branch including the required fixes and several package updates. You can find it here: https://github.com/deinsoftware/swpm/tree/package-updates Please note that the recommended minimum version of Node.js for this branch is Node 20. I would appreciate your help testing these changes. Please follow these steps:
The npm unlink
npm install -g swpmPlease let me know if everything works as expected. If you encounter any errors, please provide any evidence or logs if possible. Thanks for the help! |
Reuse existing detectOs() from open.ts instead of maintaining separate platform === 'win32' check in cmds.ts.
Add integration test that verifies swpm install works on all active LTS node versions (18, 20, 22, 24). Versions are fetched dynamically from nodejs.org API. Create .tool-versions to pin current node version for the project.
Document findings from testing the upstream package-updates branch: - detectOs refactor already included (improved version) - Build and CLI commands work correctly - 496/501 tests pass (5 timeout failures are environmental) - Compatible with all active LTS Node versions (18, 20, 22, 24) - Significant dependency updates (TypeScript 6, vitest 4, etc.) - New swpm status command added
Set 120s per-test timeout since npm install under different node versions can take longer than the default 5s, especially on first run when mise needs to download and install the node binary.
…fix-deprecation-warnings-node24 # Conflicts: # package.json # src/helpers/cmds.ts # src/helpers/open.ts
I now tested with several node versions and did not detect any issue. I also created a test script for this src/node-versions.test.ts - which actually might be improved, but at least is does some basic checks. It uses the |
| spinnies.succeed(urlId) | ||
| exit(0) | ||
| } catch (error) { | ||
| await spinnies.fail(urlId) |
There was a problem hiding this comment.
Should we keep the await on spinnies.fail(urlId)? I'm concerned that removing it might cause a race condition where the process exits before the failure state is fully rendered. Could you double-check if the UI still displays correctly before the termination?
There was a problem hiding this comment.
when committing, my sonar lint extension complained about this saying that spinnies.fail() does not return a Promise, so await has no effect and is misleading. Looking at the typing, sonar is right, so if the typing is correct, the await should not be necessary.
| } | ||
| console.error(stripIndents` | ||
| ${chalk.red.bold('Error')}: no compatible browser ${chalk.bold(`${!browserId ? browserId : ' '}`)}found. | ||
| ${chalk.red.bold('Error')}: no compatible browser ${chalk.bold(`${browserId ?? ' '}`)}found. |
There was a problem hiding this comment.
The use of ?? is much cleaner than the previous ternary operator. However, if browserId is an empty string (""), it will skip the default space. Should we use browserId || ' ' instead to handle empty strings as well? Or are we certain browserId will only be null or undefined?
There was a problem hiding this comment.
Ah, actually here I was a bit confused, as the original code ${!browserId ? browserId : ' '} would return the browserId if it is falsy. Not sure if this was intended.
And if the ID is an empty string, we have a space already before the ${}.
And now looking at the code once again, isn't `${!browserId ? browserId : ' '}` identical to !browserId ? browserId : ' '?
So maybe browserId || ' ' is a good choice 😄
There was a problem hiding this comment.
I looked at this part again: I actually assume that in case there is a browserId, that there should be a space appended, so I adjusted the code accordingly.
|
@ptandler thanks a lot for your interest and all the help with these improvements! I've left a few comments to polish some details and ensure everything is solid. I'm looking forward to your feedback so we can merge these changes and release the official version as soon as possible. Great job |
…fix-deprecation-warnings-node24
Glad I can help 🙏 Feel free to adjust my suggestions as you prefer, so you don't need to wait until I respond ... which sometimes can take a bit ... ⏳ 😴 |
| return version.includes('wsl') && version.includes('microsoft') | ||
| } | ||
|
|
||
| /** returns node platform and 'wsl' if running in wsl */ |
There was a problem hiding this comment.
Thanks for the explanation. Following Clean Code principles, I'd prefer to remove these comments. The code should be self-explanatory enough that they aren't necessary. If the logic is too complex, let's look into refactoring it instead
| /** returns node platform and 'wsl' if running in wsl */ | ||
| export const detectOs = () => { | ||
| let os = platform().toLowerCase().replace(/\d/g, '') | ||
| let os = platform().toLowerCase().replaceAll(/\d/g, '') |
There was a problem hiding this comment.
Could you explain the reasoning behind switching from .replace(/\d/g, '') to .replaceAll(/\d/g, '')? Since the original regex already uses the global flag, both should behave the same way. Is there a specific benefit you're looking for?
There was a problem hiding this comment.
this was a recommendation from sonarlint and I try to fix issues that sonar mentions before committing:
Strings should use "replaceAll()" instead of "replace()" with global regex
typescript:S7781
This rule flags when String#replace() is used with a global regex pattern, or when String#replaceAll() is used with a global regex flag.
The String#replaceAll() method was introduced in ES2021 to provide a clearer and safer way to replace all occurrences of a pattern in a string.
When using String#replace() with a global regex, developers must remember to include the global flag (g) and properly escape special regex characters if the search pattern contains them. This can lead to bugs when special characters are not escaped correctly.
For example, if you want to replace all dots in a string, using string.replace(/./g, '') will actually replace all characters (since . matches any character in regex), not just literal dots. You would need string.replace(/./g, '') instead.
With String#replaceAll(), you can simply use string.replaceAll('.', '') which is both safer and more readable. The method name clearly indicates that all occurrences will be replaced.
When String#replaceAll() is used with a regex, the global flag is required, or a TypeError will be thrown.
What is the potential impact?
Using the wrong replacement method can lead to unexpected behavior:
Incorrect replacements when special regex characters are not properly escaped
Performance issues due to unnecessary regex compilation
Reduced code readability and maintainability
Potential security vulnerabilities if user input is used in regex patterns without proper escaping
How to fix?
Replace String#replace() with global regex with String#replaceAll() using a string literal when the pattern doesn’t need regex features.
|
@equiman you can have a look again now :) |
Closes #108