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

Add Type Information for Bidi JS #12

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

yasarsid
Copy link

@yasarsid yasarsid commented Jan 21, 2025

Adding Types for Bidi JS

Overview

In this PR we are adding type information for Bidi JS. This aims to fix the issue #11.
There are a few opinion based choices - would love a review there.

Summary of the changes

  1. Added typescript as a dev dependency and generate the types as part of the build. The types are generated from the bidi.js and bidi.mjs files.
  2. In the rollup.config removed the default function of bidiFactory, added it as a named export in index.js. It is still the top level encapsulation. I felt like the bidi factory should be defined as part of the code instead of the configuration. (This is one of the opinion based decisions)
  3. This might a breaking change - so updated the major version in package.json.
  4. Made changes in the test to consider a named export instead of a default export

Other minor changes

  1. There were a few crashes in the tests, added some additional checks to ensure that the tests run.
  2. In package-lock.json fixed for known vulnerbilities.

Validations

  • Ran the build and tests locally
  • validated the output bidi.js and bidi.d.ts, included them in a local repo to check if the correct types were picked.

Artefacts

Attaching the generate d.ts, since its in the dist/ it is ignored.

export const __esModule: boolean;
export function bidiFactory(): {
    getEmbeddingLevels: (string: string, baseDirection?: "ltr" | "rtl" | "auto") => {
        paragraphs: {
            start: any;
            end: any;
            level: any;
        }[];
        levels: Uint8Array;
    };
    getReorderSegments: (string: string, embeddingLevelsResult: {
        paragraphs: {
            start: any;
            end: any;
            level: any;
        }[];
        levels: Uint8Array;
    }, start?: number, end?: number) => number[][];
    getReorderedIndices: (string: string, embedLevelsResult: {
        paragraphs: {
            start: any;
            end: any;
            level: any;
        }[];
        levels: Uint8Array;
    }, start?: number, end?: number) => number[];
    getReorderedString: (string: string, embedLevelsResult: {
        paragraphs: {
            start: any;
            end: any;
            level: any;
        }[];
        levels: Uint8Array;
    }, start?: number, end?: number) => string;
    getBidiCharType: (char: string) => number;
    getBidiCharTypeName: (char: any) => any;
    getMirroredCharacter: (char: any) => any;
    getMirroredCharactersMap: (string: any, embeddingLevels: any, start?: any, end?: any) => Map<number, string>;
    closingToOpeningBracket: (char: any) => any;
    openingToClosingBracket: (char: any) => any;
    getCanonicalBracket: (char: any) => any;
};

@yasarsid
Copy link
Author

yasarsid commented Feb 7, 2025

@lojjic - checking in to see if you missed the notification and if you generally agree with the direction

Copy link
Owner

@lojjic lojjic left a comment

Choose a reason for hiding this comment

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

Thanks for the reminder.

I made a few inline comments, but overall can we please limit this to just the types? Other things I'm happy to discuss as separate issues.

As for the types, I'm ok with the autogeneration if that continues to work, but I'd prefer not to publish types with so many anys. You should be able to add more specific types in the JSDocs it's pulling from, I think?

If the autogeneration doesn't work, I'd also be fine with a static .d.ts file in the sources that's manually maintained.

import { closingToOpeningBracket, openingToClosingBracket, getCanonicalBracket } from './brackets.js'

export function bidiFactory() {
return {
Copy link
Owner

Choose a reason for hiding this comment

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

This may seem like a safe change but it's not -- bidiFactory must contain all the code, not reference anything outside of it, so that it can be stringified and passed to web workers as a single self-contained unit. Adding it as a final wrapper in the build process ensured that was the case.

@@ -1,15 +1,17 @@
{
"name": "bidi-js",
"version": "1.0.3",
"version": "2.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Don't manually rev the build please, that's done during the publish process.

@@ -17,8 +17,15 @@ module.exports.runBidiCharacterTest = function (bidi) {
let totalTime = 0

lines.forEach((line, lineIdx) => {
if (line && line.startsWith('#')) {
return
}
Copy link
Owner

Choose a reason for hiding this comment

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

This seems redundant, is there a reason for it?

if (line && !line.startsWith('#')) {
let [input, paraDir, , expectedLevels, expectedOrder] = line.split(';')
if (!input || !paraDir || !expectedLevels || !expectedOrder) {
return
}
Copy link
Owner

Choose a reason for hiding this comment

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

Is this fixing an actual issue, or just being defensive?

import bidiFactory from 'bidi-js'
// or: const bidiFactory = require('bidi-js')

import { bidiFactory } from 'bidi-js'
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer named exports myself nowadays, but unless there's a strong reason for it I'd rather not introduce a breaking change just for an opinion. I might be open to adding a named export but keeping the default export so it's not breaking, but let's discuss that as a separate issue please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants