Add computed fields to SQL

[mohamedsalem401]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: d97a9a6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[olayway]

Good start but can be improved ;)

[olayway]

Suggested change

	export function getUniqueProperties(objects: any[]): string[] {
	const uniqueProperties: string[] = [];
	for (const object of objects) {
	for (const key of Object.keys(object)) {
	if (!uniqueProperties.includes(key)) {
	uniqueProperties.push(key);
	}
	}
	}
	return uniqueProperties;
	}
	export function getUniqueProperties(objects: any[]): string[] {
	const uniqueProperties = new Set<string>();
	for (const object of objects) {
	Object.keys(object).forEach(key => uniqueProperties.add(key));
	}
	return Array.from(uniqueProperties);
	}

[olayway]

Don't do this... It's not necessary at all. If you want to be able to assign dynamic properties I think you can just add this to the class:

  [key: string]: any

[mohamedsalem401]

But this way we lose tract of all fields, it's like using any as a type. resulting in a loss of type safety.

[olayway]

That's not what I meant. Use original properties + this one for dynamic ones (aka computed fields)

[olayway]

I think this could be much simpler:

Suggested change

	this.file._id = file._id;
	this.file.file_path = file.file_path;
	this.file.extension = file.extension;
	this.file.url_path = file.url_path;
	this.file.filetype = file.filetype;
	this.file.metadata = file.metadata ? JSON.parse(file.metadata) : null;
	// Assign dynamic properties using index signature to this.file
	for (const key in file) {
	if (
	key !== "_id" &&
	key !== "file_path" &&
	key !== "extension" &&
	key !== "url_path" &&
	key !== "filetype" &&
	key !== "metadata"
	) {
	this.file[key] = file[key];
	}
	}
	Object.keys(file).forEach(key => {
	if (key === "metadata") {
	this[key] = file[key] ? JSON.parse(file[key]) : null;
	} else {
	this[key] = file[key];
	}
	});

[mohamedsalem401]

I agree

[olayway]

What happens if a computed property returns an array or a boolean? Shouldn't it be table.<some type>? Also, were the comments incorrect?

Suggested change

	for (let index = 0; index < properties.length; index++) {
	table.string(properties[index]);
	}
	properties.forEach((property) => {
	table.string(property);
	})

[mohamedsalem401]

I see what you are saying, I thought about a couple of scenarios:

• a field is array => there's no way to store arrays in SQL
• a field is a number => what type of number? float, double, ...
• what if a field is an object => there's no way to store objects in SQL
• what if a field has a different type ( number/string ) across two files, should it be defaulted to string?

[olayway]

Well, use any SQL types that are most suited for each JS/TS type. But it's not like everything needs to be stringified 😅 If it's an object/array, stringify to JSON.

what if a field has a different type ( number/string ) across two files, should it be defaulted to string?

You could type the computed field function signature in a way that it always should return a single type defined by the user. But for now, ok, just stringify everything. We'll properly implement it when we move computed fields definitions to their final destination = document types.

[mohamedsalem401]

Yes, but the issue is currently computedFields don't return any thing.

the current implementation of ComputedField is unpredictable as the user changes the fileObject directly...

[olayway]

Rather:

Suggested change

	[key: string]: string | null | MetaData | undefined;
	[key: string]: unknown;