Skip to content

Conversation

@Juknum
Copy link
Collaborator

@Juknum Juknum commented Aug 22, 2025

Let's stop messing with JS and use TS instead so we don't have to maintain d.ts files

TODO:

  • Add build script in package.json

    • main is set to dist/index.js instead of src/index.js
    • types is set to dist/index.d.ts instead of typings/index.d.ts
      • delete old typings/index.d.ts
  • Bump version to 2.0.0

  • Update README

  • Rewrite index.js in TS and split it into multiple files

    • Make generated documentation work again
    • _address & _token & setup methods → settings.ts
    • __extract_datautils.ts in extractData
    • firestorm namespace
      • files
      • collection class
        • sha1()
        • get()
        • searchKeys()
        • search()
        • readRaw()
          • read_raw() deleted (deprecated for a while)
        • writeRaw()
          • write_raw() deleted (same as above)
        • select()
        • values(), working but not yet typed properly (need help)
        • random()
        • add()
        • addBulk()
        • remove()
        • removeBulk()
        • set()
        • setBulk()
        • editField()
        • editFieldBulk()
  • Update tests

    • Avoid regression by successfully running JS tests on compiled TS
      • tests-files.spec.mjs
      • tests-get.spec.mjs
      • tests-post.spec.mjs
      • tests-setup.spec.mjs
    • Rewrite tests in TS once JS tests are working (using Vitest)
  • Make sure the library still works in a browser

  • Make sure methods are properly added to the Collection item

    • Add tests (there is currently no tests checking this?)

Breaking changes:

  • Collection class now requires up to two generic types instead of 1:
// before:
interface Family {
	[firestorm.ID_FIELD]: string;
	parents: User[];
	children: User[];
	getDad(): Promise<User>;
	getMom(): Promise<User>;
}

firestorm.collection<Family>("families", (el) => {
	el.getDad = (): Promise<User> =>
		users.search([
			{ field: "family", criteria: "==", value: el[firestorm.ID_FIELD] },
			{ field: "sex", criteria: "==", value: "male" },
		])[0];

        // NO DEFINITIONS FOR getMom ?!

	return el;
});
// after:
interface Family {
	[firestorm.ID_FIELD]: string;
	parents: User[];
	children: User[];
}

interface FamilyMethods {
	getDad(): Promise<User>;
	getMom(): Promise<User>;
}

// type error if getMom declaration is missing
firestorm.collection<Family, FamilyMethods>("families", (el) => ({
	getDad: (): Promise<User> =>
		users.search([
			{ field: "family", criteria: "==", value: el[firestorm.ID_FIELD] },
			{ field: "sex", criteria: "==", value: "male" },
		])[0],

	getMom: (): Promise<User> =>
		users.search([
			{ field: "family", criteria: "==", value: el[firestorm.ID_FIELD] },
			{ field: "sex", criteria: "==", value: "female" },
		])[0],
}));
  • firestorm.address returns _address instead of _address + "get.php", which shouldn't be that much of an issue as firestorm resolves _address as the /get.php endpoint

@Juknum Juknum requested a review from TheRolfFR August 22, 2025 00:09
@Juknum Juknum self-assigned this Aug 22, 2025
@Juknum Juknum added enhancement New feature or request help wanted Extra attention is needed javascript Pull requests that update javascript code labels Aug 22, 2025
@Juknum
Copy link
Collaborator Author

Juknum commented Aug 22, 2025

That's weird that the pipeline doesn't fully fail when the tests doesn't pass 🤔

@TheRolfFR
Copy link
Owner

That's weird that the pipeline doesn't fully fail when the tests doesn't pass 🤔

it's because the js:test and php:test dont use the mocha return value compared to the tests in docker container

https://github.com/TheRolfFR/firestorm-db/pull/28/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R12

@TheRolfFR
Copy link
Owner

The breaking change doesn't have to be one, would it be possible to add a method called .add_methods<FamilyMethods>(...) which result in a type which is the union of both interfaces ? A deprecation message warning for the "legacy" way could do the trick. The people implementing should be conscious enough to make sure not to forget to implement methods, or it will break duh.

Start by making sure all tests run in js, then you can consider changing them in TS.
Rewriting tests in TS is relevant to make sure types compile accordingly.
Does it compile if you miss some methods implemented?

The weird axios import was made to have a single js file you could import for browser and before you have a <script src="path/to/my/axios.min.js" />, I don't know if it is still possible in ts.

for .values(...) method, just take what you have in types, no?

Copy link
Owner

@TheRolfFR TheRolfFR left a comment

Choose a reason for hiding this comment

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

I won't review the js code itself, tests will, it's now some TDD

import { GET, ID_FIELD_NAME, token, POST } from "./settings.js";
import { extractData } from "./utils.js";

import type { SearchOption } from "./types/searchOption.js";
Copy link
Owner

Choose a reason for hiding this comment

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

For type separation, consult @3vorp on #27

@Juknum Juknum force-pushed the typescript branch 5 times, most recently from 52bd803 to 1fe0cfe Compare August 22, 2025 21:07
* @param req - The request promise to extract data from.
* @returns The extracted data.
*/
export async function extractData<R>(req: Promise<any> | any): Promise<R> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export async function extractData<R>(req: Promise<any> | any): Promise<R> {
export async function extractData<T>(req: Promise<T> | T): Promise<Awaited<T>> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Awaited recursively unwraps promises so it's safe to make types with

@3vorp
Copy link
Contributor

3vorp commented Aug 23, 2025

I think the php and typescript folders in src should be renamed server/ and client/ to more accurately express intent

* @param methods - Optional methods to add to the collection elements when retrieved.
*/
constructor(
private readonly name: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

possibly unintentionally breaking change—current firestorm calls this field collectionName and it's also public readonly rather than private (no harm in accessing it)

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

Labels

enhancement New feature or request help wanted Extra attention is needed javascript Pull requests that update javascript code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants