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.

Sign up for GitHub

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

Jump to bottom

chore(web): move common/web/utils → web/src/engine/common/utils/ 🏗️ #12034

Closed
wants to merge 1 commit into from
Closed

chore(web): move common/web/utils → web/src/engine/common/utils/ 🏗️ #12034

wants to merge 1 commit into from

Conversation

ermshiperete
Copy link
Contributor

Move files from common/web/utils according to the document linked in #11374.

Fixes: #12025

@keymanapp-test-bot skip

@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Jul 26, 2024 •
edited
Loading

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S7 milestone Jul 26, 2024
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch from 92a8d4f to 3bb3bfc Compare July 26, 2024 14:37
@ermshiperete ermshiperete linked an issue Jul 26, 2024 that may be closed by this pull request
chore(web): common/web/utils/ → web/src/engine/common/utils/ (module @keymanapp/web-utils) #12025
Closed
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch 2 times, most recently from 785505d to 51bf3e9 Compare July 26, 2024 16:48
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch 2 times, most recently from e0144aa to 5b5b00f Compare July 26, 2024 18:55
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch from 5b5b00f to ac9c703 Compare July 26, 2024 19:18
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch from ac9c703 to e9ecf45 Compare July 29, 2024 15:48
@ermshiperete
chore(web): move common/web/utils → web/src/engine/common/utils/
bf963e8 
Move files from `common/web/utils` according to the document linked in
#11374.

Fixes: #12025
@ermshiperete ermshiperete force-pushed the chore/web/11374_restructure branch from e9ecf45 to bf963e8 Compare July 30, 2024 09:43
@ermshiperete ermshiperete changed the title chore(web): move common/web/utils → web/src/engine/common/utils/ chore(web): move common/web/utils → web/src/engine/common/utils/ 🏗️ Jul 30, 2024
@ermshiperete ermshiperete marked this pull request as ready for review July 30, 2024 13:58
mcdurdin
mcdurdin approved these changes Jul 30, 2024
View reviewed changes
common/models/templates/build.sh
@@ -16,7 +16,7 @@ builder_describe "Builds the predictive-text model template implementation modul
"@/common/web/keyman-version" \
"@/common/web/es-bundling" \
"@/common/models/wordbreakers" \
"@/common/web/utils" \
"@/web/src/engine/common/utils" \
Copy link
Member

Choose a reason for hiding this comment

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

Note: this is only going to be okay if we are also moving common/models/templates over, otherwise we are introducing a deep dep from common -> web.

Copy link
Contributor

@jahorton jahorton Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also of note: some of my current predictive-text work is likely to see developer/src/kmc-model import from common/models/templates.

Copy link
Contributor

@jahorton jahorton Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

From #11994 (currently, WIP):

https://github.com/keymanapp/keyman/blob/5011d7fed68f79e0368f42471d2c74bc7742556d/common/models/templates/src/trie-builder.ts pulls in the primary Trie-construction code currently found in kmc-model. We'll likely want it for user-dictionary support.

Accordingly...

keyman/developer/src/kmc-model/src/build-trie.ts

Lines 1 to 4 in 5011d7f

import { ModelCompilerError, ModelCompilerMessageContext, ModelCompilerMessages } from "./model-compiler-messages.js";
import { callbacks } from "./compiler-callbacks.js";
import { TrieBuilder } from '@keymanapp/models-templates';

Copy link
Contributor

@jahorton jahorton Jul 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In a similar vein, #11088 will want to affect the same section of code (i.e, trie building), adding better Trie compression strategies. The compression part could be kept kmc-model only in theory, but the decompression part will certainly be needed within the live model - and personally, it makes the most sense if the two parts can be tested together in parallel. My current plan is to place the definition for both within common/models/templates.

Copy link
Member

Choose a reason for hiding this comment

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

OK, that's a bad dependency chain we are setting up for ourselves there -- developer/src/kmc-model -> common/models/templates -> web/src/engine/common/utils.

I think we need to discuss this more before we continue moving things around, because we are treading on each others' toes a bit, and it's liable to become (more of) a mess if we are not careful!

@ermshiperete
Copy link
Contributor Author

We can't move common/model/templates since it's used by both Web and Developer, so I'm closing this PR.

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

@mcdurdin mcdurdin mcdurdin approved these changes

@jahorton jahorton Awaiting requested review from jahorton

Assignees
No one assigned
Labels
chore common/models/templates/ common/models/ common/resources/ Build infrastructure common/web/ common/ web/
Projects
None yet
Milestone
A18S7
Development

Successfully merging this pull request may close these issues.

chore(web): common/web/utils/ → web/src/engine/common/utils/ (module @keymanapp/web-utils)
3 participants
@ermshiperete @mcdurdin @jahorton