Add instructions for using this ext in dev of another#2197
Add instructions for using this ext in dev of another#2197imnasnainaec wants to merge 2 commits intodevelopfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds documentation for integrating the extension with another development extension and updates the service registration key naming convention from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
imnasnainaec
force-pushed
the
pb-ext-readme-drop-into-paranext-core
branch
from
c5fcd78 to
613cdfa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform.bible-extension/README.md`:
- Around line 163-181: The doc shows the consumer calling
papi.networkObjects.set and importing EntryService which incorrectly
re-registers a provider; instead instruct the consumer to retrieve the existing
service with await
papi.networkObjects.get<IEntryService>('fwLiteExtension.entryService') (so they
do not import EntryService or hard-code the URL) and remove the code that adds
the returned entryService to context.registrations (i.e., do not call
context.registrations.add(await entryService)); mirror the usage in
src/web-views/add-word.web-view.tsx and reference the symbols
papi.networkObjects.get, papi.networkObjects.set, fwLiteExtension.entryService,
EntryService, IEntryService, and context.registrations.add when locating the
lines to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: eaca491d-f7bf-439e-9a6d-c7db93f99d00
📒 Files selected for processing (6)
platform.bible-extension/README.mdplatform.bible-extension/src/main.tsplatform.bible-extension/src/services/entry-service.tsplatform.bible-extension/src/web-views/add-word.web-view.tsxplatform.bible-extension/src/web-views/find-related-words.web-view.tsxplatform.bible-extension/src/web-views/find-word.web-view.tsx
platform.bible-extension/README.md
Outdated
| 3. In the other extension's `main.ts`, add the following two things inside `activate`: | ||
|
|
||
| ```ts | ||
| const entryService = papi.networkObjects.set( | ||
| 'fwLiteExtension.entryService', | ||
| new EntryService('http://localhost:29348'), // TODO: avoid hard-coding the base URL | ||
| 'fw-lite-extension.IEntryService', | ||
| ); | ||
| ``` | ||
|
|
||
| ```ts | ||
| context.registrations.add( | ||
| /* ... along with other unsubscribers ... */ | ||
|
|
||
| await entryService, | ||
| ); | ||
| ``` | ||
|
|
||
| 4. An example of using this network service is found in [add-word.web-view.tsx](src/web-views/add-word.web-view.tsx). |
There was a problem hiding this comment.
Document consumption with get(...), not provider registration.
After step 2, the installed fw-lite-extension already owns fwLiteExtension.entryService from its own activate(). Having the other extension call networkObjects.set(...) again turns the consumer into a second provider, can conflict with the installed extension, and requires importing the internal EntryService implementation. This example should mirror src/web-views/add-word.web-view.tsx and retrieve the existing service instead.
📝 Suggested doc fix
-3. In the other extension's `main.ts`, add the following two things inside `activate`:
+3. In the other extension, retrieve the existing network service instead of registering a new one:
```ts
-const entryService = papi.networkObjects.set(
- 'fwLiteExtension.entryService',
- new EntryService('http://localhost:29348'), // TODO: avoid hard-coding the base URL
- 'fw-lite-extension.IEntryService',
+const entryService = await papi.networkObjects.get<IEntryService>(
+ 'fwLiteExtension.entryService',
);-```ts
-context.registrations.add(
- /* ... along with other unsubscribers ... */
- await entryService,
-);
-```
- An example of using this network service is found in add-word.web-view.tsx.
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
3. In the other extension, retrieve the existing network service instead of registering a new one:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@platform.bible-extension/README.md` around lines 163 - 181, The doc shows the
consumer calling papi.networkObjects.set and importing EntryService which
incorrectly re-registers a provider; instead instruct the consumer to retrieve
the existing service with await
papi.networkObjects.get<IEntryService>('fwLiteExtension.entryService') (so they
do not import EntryService or hard-code the URL) and remove the code that adds
the returned entryService to context.registrations (i.e., do not call
context.registrations.add(await entryService)); mirror the usage in
src/web-views/add-word.web-view.tsx and reference the symbols
papi.networkObjects.get, papi.networkObjects.set, fwLiteExtension.entryService,
EntryService, IEntryService, and context.registrations.add when locating the
lines to change.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
I love how simple and elegant this looks 👍
Looks like you found just the right way to do this.
|
|
||
| export class EntryService implements IEntryService { | ||
| private fwLiteApi: FwLiteApi; | ||
| // TODO: remove the need to specify baseUrl, so other extensions can use this network service. |
There was a problem hiding this comment.
❓ I'm slightly puzzled by this TODO. It sounds like a good idea to me, but it also sounds like hard-coding the baseUrl is currently a working solution, whereas the comment suggests that other extensions can't use the service as long as the baseUrl needs to be specified.
| 'fwLiteExtension.entryService', | ||
| new EntryService('http://localhost:29348'), // TODO: avoid hard-coding the base URL | ||
| 'fw-lite-extension.IEntryService', |
There was a problem hiding this comment.
❓ The naming inconsistency here is a bit curious. It looks like we're specifying the object ID and the object type. So, they're fairly different. But maybe it's still desirable to still standardize the "namespace" to e.g. fw-lite-extension?
imnasnainaec
force-pushed
the
pb-ext-readme-drop-into-paranext-core
branch
from
613cdfa to
fc94842
Compare
|
Dropping a bug note here for future investigation: Sometimes, even when the extension is generally working (e.g., the "Search in FieldWorks..." menu item opens and functions), the "Browse FieldWorks dictionary can fail" with the following in the tab it opens:
UPDATE: This is when copying the release |
2 participants
No description provided.