Skip to content

Misc types fixes #7618

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

Merged
merged 6 commits into from
Apr 28, 2025
Merged

Misc types fixes #7618

merged 6 commits into from
Apr 28, 2025

Conversation

willeastcott
Copy link
Contributor

@willeastcott willeastcott commented Apr 27, 2025

  • Remove string.fromCodePoint from public API. It's just a polyfill for the now well-supported String.fromCodePoint and should be considered just for internal use.

Fixes #7616

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

@willeastcott willeastcott self-assigned this Apr 27, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes miscellaneous type fixes by updating documentation and modernizing parameter handling.

  • Removed the outdated "system" parameter documentation from the ScrollViewComponent's _toggleLifecycleListeners method.
  • Updated the fromCodePoint utility function to use rest parameters instead of the arguments object.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/framework/components/scroll-view/component.js Removed outdated documentation for the unused "system" parameter.
src/core/string.js Refactored fromCodePoint to simplify parameter handling using rest parameters.
Comments suppressed due to low confidence (1)

src/framework/components/scroll-view/component.js:603

  • The documentation for the 'system' parameter was removed, which is fine if the method signature no longer requires it. Please confirm that this change aligns with the updated implementation and that any related logic has been adjusted accordingly.
* @param {ScrollViewComponentSystem} system - The ComponentSystem that created this Component.

@kungfooman
Copy link
Collaborator

Since it's a compatibility function, I wrote some test code of your version, and both work, since it's nearly no change at all, just removing this crude polyfill look:

/**
 * Get the string for a given code point or set of code points. Polyfill for
 * [`fromCodePoint`]{@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/fromCodePoint}.
 *
 * @param {...number} args - The code points to convert to a string.
 * @returns {string} The converted string.
 */
function fromCodePoint(...args) {
    const chars = [];
    let current;
    let codePoint;
    let units;
    for (let i = 0; i < args.length; ++i) {
        current = Number(args[i]);
        codePoint = current - 0x10000;
        units = current > 0xFFFF ? [(codePoint >> 10) + 0xD800, (codePoint % 0x400) + 0xDC00] : [current];
        chars.push(String.fromCharCode.apply(null, units));
    }
    return chars.join('');
}
// String: ABC
console.log(String.fromCodePoint(65, 66, 67) === fromCodePoint(65, 66, 67));
// String: "☃★♲你"
console.log(String.fromCodePoint(9731, 9733, 9842, 0x2f804) === fromCodePoint(9731, 9733, 9842, 0x2f804));

I would clean up the rest aswell, it's clearly const and this .apply is just an ES5 artifact:

/**
 * Get the string for a given code point or set of code points. Polyfill for
 * [`fromCodePoint`]{@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/fromCodePoint}.
 *
 * @param {...number} args - The code points to convert to a string.
 * @returns {string} The converted string.
 */
function fromCodePoint(...args) {
    const chars = [];
    for (let i = 0; i < args.length; ++i) {
        const current = Number(args[i]);
        const codePoint = current - 0x10000;
        const units = current > 0xFFFF ? [(codePoint >> 10) + 0xD800, (codePoint % 0x400) + 0xDC00] : [current];
        chars.push(String.fromCharCode(...units));
    }
    return chars.join('');
}
// String: ABC
console.log(String.fromCodePoint(65, 66, 67) === fromCodePoint(65, 66, 67));
// String: "☃★♲你"
console.log(String.fromCodePoint(9731, 9733, 9842, 0x2f804) === fromCodePoint(9731, 9733, 9842, 0x2f804));

Otherwise LGTM, great and quick work 👍

Copy link
Collaborator

@kungfooman kungfooman left a comment

Choose a reason for hiding this comment

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

💯

@willeastcott willeastcott requested a review from Copilot April 28, 2025 09:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the internal polyfill implementation of string.fromCodePoint and cleans up associated documentation while adding more extensive tests for character code conversion.

  • Updated the string.fromCodePoint implementation to use modern spread/rest parameters and added an @ignore tag.
  • Expanded test coverage for string.fromCodePoint to include various Unicode scenarios.
  • Removed an unused JSDoc parameter from the scroll-view component.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/core/string.test.mjs Added tests for string.fromCodePoint, but accidentally left a .only
src/framework/components/scroll-view/component.js Removed redundant JSDoc parameter details
src/core/string.js Revised fromCodePoint implementation and updated documentation

@willeastcott willeastcott merged commit d96c675 into main Apr 28, 2025
7 checks passed
@willeastcott willeastcott deleted the type-fixes branch April 28, 2025 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Last two type issues (scroll view component + core/string fromCodePoint)
3 participants