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

refactor(developer): use npm or local source for server addons instead of github references #12090

Merged
merged 10 commits into from
Aug 14, 2024

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Aug 2, 2024

Brings trayicon source into the repository, as the source repo appears to be dead. Updates console source to latest version on npm.

Opted not to bring the source into the repo rather than keep in a separate repo because npm doesn't behave well with git/https links, and we don't want to publish yet another package.

Fixes: #12077

User Testing

  • TEST_SERVER: In Keyman Developer, verify that Keyman Developer Server is working correctly.
  • TEST_SERVER_TRAY_ICON: In Keyman Developer, verify that the Server icon is accessible in the System Notification Area.
  • TEST_SERVER_CONSOLE_WINDOW: In Keyman Developer, verify that Hide/Show Console window functions work in the Server icon menu.

@keymanapp-test-bot keymanapp-test-bot bot added has-user-test user-test-required User tests have not been completed labels Aug 2, 2024
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Aug 2, 2024

User Test Results

Test specification and instructions

Test Artifacts

@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S7 milestone Aug 2, 2024
@darcywong00 darcywong00 modified the milestones: A18S7, A18S8 Aug 2, 2024
@mcdurdin mcdurdin marked this pull request as ready for review August 5, 2024 13:51
@mcdurdin mcdurdin requested a review from darcywong00 as a code owner August 5, 2024 13:51
@mcdurdin mcdurdin linked an issue Aug 5, 2024 that may be closed by this pull request
Base automatically changed from refactor/common/9665-move-compiler-interfaces-to-developer to master August 7, 2024 05:15
@mcdurdin
Copy link
Member Author

mcdurdin commented Aug 8, 2024

Server is failing to start on the build associated with 38d043c:

C:\Program Files (x86)\Keyman\Keyman Developer\Server>..\node.js\node.exe .
node:internal/errors:490
    ErrorCaptureStackTrace(err);
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@keymanapp/common-types' imported from C:\Program Files (x86)\Keyman\Keyman Developer\Server\node_modules\@keymanapp\developer-utils\build\src\types\kpj\kpj-file-reader.js
    at new NodeError (node:internal/errors:399:5)
    at packageResolve (node:internal/modules/esm/resolve:889:9)
    at moduleResolve (node:internal/modules/esm/resolve:938:20)
    at defaultResolve (node:internal/modules/esm/resolve:1153:11)
    at nextResolve (node:internal/modules/esm/loader:163:28)
    at ESMLoader.resolve (node:internal/modules/esm/loader:838:30)
    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:77:40)
    at link (node:internal/modules/esm/module_job:76:36) {
  code: 'ERR_MODULE_NOT_FOUND'
}

Node.js v18.14.1

@dinakaranr
Copy link

Test Results

I tested the PR #12090 task today using the Keyman 18.0.81-alpha-test-12090 build. I repeatedly get the confirm dialog after clicking the "Test Keyboard on Web" button. Hence, I am unable to launch the keyboard on the browser (localhost:8008). I observed this behavior on Windows 10 and 11 OSs. So, I am unable to proceed with the testing. Hence, I failed this PR. Thank you.

  • TEST_SERVER (Failed):
  • TEST_SERVER_TRAY_ICON (Failed):
  • TEST_SERVER_CONSOLE_WINDOW (Failed):

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Aug 8, 2024
@mcdurdin mcdurdin force-pushed the refactor/developer/12077-remove-git-npm-deps branch from 99822b6 to 2fc6a5b Compare August 11, 2024 23:08
@mcdurdin mcdurdin force-pushed the refactor/developer/12077-remove-git-npm-deps branch from 2fc6a5b to 2814ac9 Compare August 11, 2024 23:09
@mcdurdin
Copy link
Member Author

Note: force push to cleanup commit history on developer/src/kmc/test/fixtures/get-last-git-commit-date/README.md, which was involved in a throwaway non-merge commit (d8986be), breaking the unit test. (Yeah, this test is a bit fragile, I guess we could mock the call to git -- which makes it a unit test rather than an integration type test -- but it seemed important to me to test that we were actually using git correctly too.)

@mcdurdin
Copy link
Member Author

Latest build:

Error [ERR_MODULE_NOT_FOUND]: Cannot find package 'restructure' imported from C:\Program Files (x86)\Keyman\Keyman Developer\server\node_modules\@keymanapp\developer-utils\build\src\types\kmx\kmx-builder.js
←[90m    at new NodeError (node:internal/errors:399:5)←[39m
←[90m    at packageResolve (node:internal/modules/esm/resolve:889:9)←[39m
←[90m    at moduleResolve (node:internal/modules/esm/resolve:938:20)←[39m
←[90m    at defaultResolve (node:internal/modules/esm/resolve:1153:11)←[39m
←[90m    at nextResolve (node:internal/modules/esm/loader:163:28)←[39m
←[90m    at ESMLoader.resolve (node:internal/modules/esm/loader:838:30)←[39m
←[90m    at ESMLoader.getModuleJob (node:internal/modules/esm/loader:424:18)←[39m
←[90m    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:77:40)←[39m
←[90m    at link (node:internal/modules/esm/module_job:76:36)←[39m {
  code: ←[32m'ERR_MODULE_NOT_FOUND'←[39m
}

The way that Server is built for local distribution does not currently
handle transitive dependencies for @keymanapp dependencies well. So,
this adds four dependencies from @keymanapp/developer-utils to Server so
that they will be available in the packaged deployment.

A future improvement could be to package developer-utils using npm
packaging tools, but these struggled with bundling monorepo dependencies
in the past also.
@mcdurdin
Copy link
Member Author

@dinakaranr I have resolved the issues that were blocking user test. Would you be able to re-test? Thanks!

@keymanapp-test-bot retest all

@keymanapp-test-bot keymanapp-test-bot bot added user-test-required User tests have not been completed and removed user-test-failed labels Aug 12, 2024
@dinakaranr
Copy link

dinakaranr commented Aug 13, 2024

Test Results

I tested this issue with the attached "Keymandeveloper-18.0.87-Alpha-test-12090" build on the Windows 10 and 11 OS environments: Here is my observation.

  • TEST_SERVER (passed): 
  1. I installed the "keymandeveloper-18.0.87.exe" file.
  2. Double-click on the "Keyman Developer" icon.
  3. The developer tool appeared correctly.
  4. Open the Keyman keyboard project (*.kpj) file.
  5. Open any keyman keyboard (*.kmn) file. (e.g., gff_amharic.kmn).
  6. Navigate to the Debugger mode by pressing the F5 or (On build tab/Start debugging).
  7. Navigate to the layout tab. The debugger works well.
  8. Click on an empty text box. I can enter some text.
  9. Click the "Test Keyboard on Web" button to start the web keyboard server.
  • TEST_SERVER_TRAY_ICON (passed): 
  1. The server icon appears in the tray. 
  2. Click on the server icon. It shows the four options. 
  • TEST_SERVER_CONSOLE_WINDOW (failed): 
  1. Click the "http://localhost:8008 in browser" option.
  2. Observe that the web keyboard opened on the browser.
  3. Click the "Show Console" option. observed that the "Power Shell" window does not open for the first click. but it is opening for the second time.
  4. Click the "Hide Console" option. I observed that the "Power Shell" window closed automatically.
  5. Click "Exit Keyman Development Server" and observe that the icon disappeared from the tray.
  6. I failed this scenario because the "Power Shell" window does not open for the first click.

@keymanapp-test-bot keymanapp-test-bot bot added user-test-failed and removed user-test-required User tests have not been completed labels Aug 13, 2024
@mcdurdin
Copy link
Member Author

  • TEST_SERVER_CONSOLE_WINDOW (failed):
  1. Click the "http://localhost:8008 in browser" option.
  2. Observe that the web keyboard opened on the browser.
  3. Click the "Show Console" option. observed that the "Power Shell" window does not open for the first click. but it is opening for the second time.
  4. Click the "Hide Console" option. I observed that the "Power Shell" window closed automatically.
  5. Click "Exit Keyman Development Server" and observe that the icon disappeared from the tray.
  6. I failed this scenario because the "Power Shell" window does not open for the first click.

Thank you for the test @dinakaranr and picking this up. It turns out that this issue is already being tracked:

As the problem is not new to this PR, and is not major I will mark this as 'SKIPPED' and go ahead and merge:

TEST_SERVER_CONSOLE_WINDOW (skipped)

@mcdurdin mcdurdin merged commit cf6ee68 into master Aug 14, 2024
6 checks passed
@mcdurdin mcdurdin deleted the refactor/developer/12077-remove-git-npm-deps branch August 14, 2024 11:21
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.89-alpha

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

Successfully merging this pull request may close these issues.

refactor(developer): bring optional deps in Server into repo
6 participants