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

MESDK-87 Add delegate pattern for MessagesQuery<->LintReportQuery communication #2734

Merged
merged 11 commits into from
May 10, 2024
5 changes: 5 additions & 0 deletions .changeset/nine-adults-dance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@inlang/sdk": patch
---

Adds delegate pattern for MessagesQuery<->LintReportQuery communication
9 changes: 9 additions & 0 deletions inlang/source-code/sdk/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ export type Subscribable<Value> = {
subscribe: (callback: (value: Value) => void) => void
}

export type MessageQueryDelegate = {
onMessageCreate: (messageId: string, message: Message) => void
onMessageUpdate: (messageId: string, message: Message) => void
onMessageDelete: (messageId: string) => void
onLoaded: (messages: Message[]) => void
onCleanup: () => void
}

export type MessageQueryApi = {
create: (args: { data: Message }) => boolean
get: ((args: { where: { id: Message["id"] } }) => Readonly<Message>) & {
Expand All @@ -101,6 +109,7 @@ export type MessageQueryApi = {
update: (args: { where: { id: Message["id"] }; data: Partial<Message> }) => boolean
upsert: (args: { where: { id: Message["id"] }; data: Message }) => void
delete: (args: { where: { id: Message["id"] } }) => boolean
setDelegate: (delegate: MessageQueryDelegate) => void
}

export type MessageLintReportsQueryApi = {
Expand Down
114 changes: 47 additions & 67 deletions inlang/source-code/sdk/src/createMessageLintReportsQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,19 @@ import type {
InstalledMessageLintRule,
MessageLintReportsQueryApi,
MessageQueryApi,
MessageQueryDelegate,
} from "./api.js"
import type { ProjectSettings } from "@inlang/project-settings"
import type { resolveModules } from "./resolve-modules/index.js"
import type { MessageLintReport, Message } from "./versionedInterfaces.js"
import { lintSingleMessage } from "./lint/index.js"
import { createRoot, createEffect } from "./reactivity/solid.js"

import { throttle } from "throttle-debounce"
import _debug from "debug"
const debug = _debug("sdk:lintReports")

function sleep(ms: number) {
return new Promise((resolve) => setTimeout(resolve, ms))
}

/**
* Creates a reactive query API for messages.
* Creates a ~~reactive~~ query API for lint reports.
*/
export function createMessageLintReportsQuery(
messagesQuery: MessageQueryApi,
Expand All @@ -42,81 +38,65 @@ export function createMessageLintReportsQuery(
}
}

const messages = messagesQuery.getAll() as Message[]

const trackedMessages: Map<string, () => void> = new Map()

debug(`createMessageLintReportsQuery ${rulesArray?.length} rules, ${messages.length} messages`)

// TODO: don't throttle when no debug
let lintMessageCount = 0
const throttledLogLintMessage = throttle(2000, (messageId) => {
debug(`lintSingleMessage: ${lintMessageCount} id: ${messageId}`)
})

createEffect(() => {
const currentMessageIds = new Set(messagesQuery.includedMessageIds())

const deletedTrackedMessages = [...trackedMessages].filter(
(tracked) => !currentMessageIds.has(tracked[0])
)

if (rulesArray) {
for (const messageId of currentMessageIds) {
if (!trackedMessages.has(messageId)) {
createRoot((dispose) => {
createEffect(() => {
const message = messagesQuery.get({ where: { id: messageId } })
if (!message) {
return
}
if (!trackedMessages?.has(messageId)) {
// initial effect execution - add dispose function
trackedMessages?.set(messageId, dispose)
}
const lintMessage = (message: Message, messages: Message[]) => {
if (!rulesArray) {
return
}

lintSingleMessage({
rules: rulesArray,
settings: settingsObject(),
messages: messages,
message: message,
}).then((report) => {
lintMessageCount++
throttledLogLintMessage(messageId)
if (report.errors.length === 0 && index.get(messageId) !== report.data) {
// console.log("lintSingleMessage", messageId, report.data.length)
index.set(messageId, report.data)
}
})
})
})
}
// TODO unhandled promise rejection (as before the refactor) but won't tackle this in this pr
lintSingleMessage({
rules: rulesArray,
settings: settingsObject(),
messages: messages,
message: message,
}).then((report) => {
if (report.errors.length === 0 && index.get(message.id) !== report.data) {
// console.log("lintSingleMessage", messageId, report.data.length)
index.set(message.id, report.data)
}
})
}

for (const deletedMessage of deletedTrackedMessages) {
const deletedMessageId = deletedMessage[0]
const messages = messagesQuery.getAll() as Message[]
// load report for all messages once
for (const message of messages) {
// NOTE: this potentually creates thousands of promisses we could create a promise that batches linting
lintMessage(message, messages)
}

// call dispose to cleanup the effect
const messageEffectDisposeFunction = trackedMessages.get(deletedMessageId)
if (messageEffectDisposeFunction) {
messageEffectDisposeFunction()
trackedMessages.delete(deletedMessageId)
// remove lint report result
index.delete(deletedMessageId)
debug(`delete lint message id: ${deletedMessageId}`)
}
const messageQueryChangeDelegate: MessageQueryDelegate = {
onCleanup: () => {
// NOTE: we could cancel all running lint rules - but results get overritten anyway
index.clear()
},
onLoaded: (messages: Message[]) => {
for (const message of messages) {
lintMessage(message, messages)
}
}
})
},
onMessageCreate: (messageId: string, message: Message) => {
lintMessage(message, messages)
},
onMessageUpdate: (messageId: string, message: Message) => {
lintMessage(message, messages)
},
onMessageDelete: (messageId: string) => {
index.delete(messageId)
},
}

messagesQuery.setDelegate(messageQueryChangeDelegate)

return {
getAll: async () => {
// TODO reintroduce reactivity here - wont be the bottleneck
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't spend more time on this TODO - let's remove it.

await sleep(0) // evaluate on next tick to allow for out-of-order effects
return structuredClone(
[...index.values()].flat().length === 0 ? [] : [...index.values()].flat()
)
},
get: async (args: Parameters<MessageLintReportsQueryApi["get"]>[0]) => {
// TODO reintroduce reactivity here
Copy link
Contributor

Choose a reason for hiding this comment

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

same - no need to re-introduce reactivity.

await sleep(0) // evaluate on next tick to allow for out-of-order effects
return structuredClone(index.get(args.where.messageId) ?? [])
},
Expand Down
48 changes: 44 additions & 4 deletions inlang/source-code/sdk/src/createMessagesQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { Message } from "@inlang/message"
import { ReactiveMap } from "./reactivity/map.js"
import { createEffect } from "./reactivity/solid.js"
import { createSubscribable } from "./loadProject.js"
import type { InlangProject, MessageQueryApi } from "./api.js"
import type { InlangProject, MessageQueryApi, MessageQueryDelegate } from "./api.js"
import type { ResolvedPluginApi } from "./resolve-modules/plugins/types.js"
import type { resolveModules } from "./resolve-modules/resolveModules.js"
import { createNodeishFsWithWatcher } from "./createNodeishFsWithWatcher.js"
Expand Down Expand Up @@ -62,6 +62,13 @@ export function createMessagesQuery({
// filepath for the lock folder
const messageLockDirPath = projectPath + "/messagelock"

// TODO check if we should use a weak ref instead
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer not to complicate the code with weak refs unless really needed.

let delegate: MessageQueryDelegate | undefined = undefined

const setDelegate = (newDelegate: MessageQueryDelegate) => {
delegate = newDelegate
}

// Map default alias to message
// Assumes that aliases are only created and deleted, not updated
// TODO #2346 - handle updates to aliases
Expand Down Expand Up @@ -98,6 +105,7 @@ export function createMessagesQuery({
onCleanup(() => {
// stop listening on fs events
abortController.abort()
delegate?.onCleanup()
})

const fsWithWatcher = createNodeishFsWithWatcher({
Expand All @@ -111,6 +119,7 @@ export function createMessagesQuery({
messageLockDirPath,
messageStates,
index,
delegate,
_settings, // NOTE we bang here - we don't expect the settings to become null during the livetime of a project
resolvedPluginApi
)
Expand All @@ -133,6 +142,7 @@ export function createMessagesQuery({
messageLockDirPath,
messageStates,
index,
delegate,
_settings, // NOTE we bang here - we don't expect the settings to become null during the livetime of a project
resolvedPluginApi
)
Expand All @@ -142,6 +152,7 @@ export function createMessagesQuery({
})
.then(() => {
onInitialMessageLoadResult()
delegate?.onLoaded([...index.values()])
})
})

Expand All @@ -165,6 +176,7 @@ export function createMessagesQuery({
messageLockDirPath,
messageStates,
index,
delegate,
_settings, // NOTE we bang here - we don't expect the settings to become null during the livetime of a project
resolvedPluginApi
)
Expand All @@ -181,6 +193,7 @@ export function createMessagesQuery({
}

return {
setDelegate,
create: ({ data }): boolean => {
if (index.has(data.id)) return false
index.set(data.id, data)
Expand All @@ -189,6 +202,7 @@ export function createMessagesQuery({
}

messageStates.messageDirtyFlags[data.id] = true
delegate?.onMessageCreate(data.id, index.get(data.id))
scheduleSave()
return true
},
Expand All @@ -215,6 +229,7 @@ export function createMessagesQuery({
if (message === undefined) return false
index.set(where.id, { ...message, ...data })
messageStates.messageDirtyFlags[where.id] = true
delegate?.onMessageCreate(where.id, index.get(data.id))
scheduleSave()
return true
},
Expand All @@ -225,10 +240,13 @@ export function createMessagesQuery({
if ("default" in data.alias) {
defaultAliasIndex.set(data.alias.default, data)
}
messageStates.messageDirtyFlags[where.id] = true
delegate?.onMessageCreate(data.id, index.get(data.id))
} else {
index.set(where.id, { ...message, ...data })
messageStates.messageDirtyFlags[where.id] = true
delegate?.onMessageUpdate(data.id, index.get(data.id))
}
messageStates.messageDirtyFlags[where.id] = true
scheduleSave()
return true
},
Expand All @@ -240,6 +258,7 @@ export function createMessagesQuery({
}
index.delete(where.id)
messageStates.messageDirtyFlags[where.id] = true
delegate?.onMessageDelete(where.id)
scheduleSave()
return true
},
Expand Down Expand Up @@ -271,6 +290,7 @@ async function loadMessagesViaPlugin(
lockDirPath: string,
messageState: MessageState,
messages: Map<string, Message>,
delegate: MessageQueryDelegate | undefined,
settingsValue: ProjectSettings,
resolvedPluginApi: ResolvedPluginApi
) {
Expand Down Expand Up @@ -339,6 +359,7 @@ async function loadMessagesViaPlugin(
messages.set(loadedMessageClone.id, loadedMessageClone)
// NOTE could use hash instead of the whole object JSON to save memory...
messageState.messageLoadHash[loadedMessageClone.id] = importedEnecoded
delegate?.onMessageUpdate(loadedMessageClone.id, loadedMessageClone)
} else {
// message with the given alias does not exist so far
loadedMessageClone.alias = {} as any
Expand All @@ -365,6 +386,7 @@ async function loadMessagesViaPlugin(
// we don't have to check - done before hand if (messages.has(loadedMessageClone.id)) return false
messages.set(loadedMessageClone.id, loadedMessageClone)
messageState.messageLoadHash[loadedMessageClone.id] = importedEnecoded
delegate?.onMessageUpdate(loadedMessageClone.id, loadedMessageClone)
}
}
await releaseLock(fs as NodeishFilesystem, lockDirPath, "loadMessage", lockTime)
Expand All @@ -388,7 +410,15 @@ async function loadMessagesViaPlugin(
messageState.sheduledLoadMessagesViaPlugin = undefined

// recall load unawaited to allow stack to pop
loadMessagesViaPlugin(fs, lockDirPath, messageState, messages, settingsValue, resolvedPluginApi)
loadMessagesViaPlugin(
fs,
lockDirPath,
messageState,
messages,
delegate,
settingsValue,
resolvedPluginApi
)
.then(() => {
// resolve the scheduled load message promise
executingScheduledMessages.resolve()
Expand All @@ -405,6 +435,7 @@ async function saveMessagesViaPlugin(
lockDirPath: string,
messageState: MessageState,
messages: Map<string, Message>,
delegate: MessageQueryDelegate | undefined,
settingsValue: ProjectSettings,
resolvedPluginApi: ResolvedPluginApi
): Promise<void> {
Expand Down Expand Up @@ -491,6 +522,7 @@ async function saveMessagesViaPlugin(
lockDirPath,
messageState,
messages,
delegate,
settingsValue,
resolvedPluginApi
)
Expand Down Expand Up @@ -530,7 +562,15 @@ async function saveMessagesViaPlugin(
const executingSheduledSaveMessages = messageState.sheduledSaveMessages
messageState.sheduledSaveMessages = undefined

saveMessagesViaPlugin(fs, lockDirPath, messageState, messages, settingsValue, resolvedPluginApi)
saveMessagesViaPlugin(
fs,
lockDirPath,
messageState,
messages,
delegate,
settingsValue,
resolvedPluginApi
)
.then(() => {
executingSheduledSaveMessages.resolve()
})
Expand Down
1 change: 0 additions & 1 deletion inlang/source-code/sdk/src/loadProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ import { identifyProject } from "./telemetry/groupIdentify.js"
import _debug from "debug"
const debug = _debug("sdk:loadProject")


const settingsCompiler = TypeCompiler.Compile(ProjectSettings)

/**
Expand Down
Loading