-
Notifications
You must be signed in to change notification settings - Fork 557
Add ODSP support to examples in /examples/apps following PR #25524 pattern #25533
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: ChumpChief <[email protected]>
Co-authored-by: ChumpChief <[email protected]>
…ison examples Co-authored-by: ChumpChief <[email protected]>
Co-authored-by: ChumpChief <[email protected]>
…t examples Co-authored-by: ChumpChief <[email protected]>
Co-authored-by: ChumpChief <[email protected]>
examples/apps/ai-collab/package.json
Outdated
| "start:server": "tinylicious" | ||
| }, | ||
| "devDependencies": { | ||
| "@fluid-example/example-webpack-integration": "workspace:~", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Revert this change, leave the ai-collab package as-is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted the ai-collab package.json changes as requested - the file is now left as-is. (568ed9b)
| try { | ||
| await start(); | ||
| } catch (error) { | ||
| console.error(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this whole try/catch block, and just await start(); here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch block and replaced with simple await start(); as requested. (568ed9b)
| try { | ||
| await start(); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.log( | ||
| "%cEnsure you are running the appropriate Fluid Server for the selected service\nUse:`npm run start:server` for local, or configure ODSP for odsp", | ||
| "font-size:30px", | ||
| ); | ||
| } | ||
| if (specifiedContact === undefined) { | ||
| // If a contact was not specified, we'll render the full collection. | ||
| const contactCollection = model.contactCollection; | ||
| renderContactCollection(contactCollection, getContactUrl, contentDiv); | ||
| } else { | ||
| // If a contact was specified, we'll render just that contact. | ||
| const contact = model.contactCollection.getContact(specifiedContact); | ||
| if (contact === undefined) { | ||
| throw new Error("Contact not found"); | ||
| } | ||
| renderContact(contact, contentDiv); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this whole try/catch block you've added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch block and cleaned up the duplicate code sections in contact-collection/src/app.ts. (568ed9b)
| } | ||
|
|
||
| try { | ||
| await start(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the try/catch here, and just await start(); instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the duplicate code issue in contact-collection - removed all try/catch blocks and replaced with simple await start();. (568ed9b)
examples/apps/staging/src/start.ts
Outdated
| try { | ||
| await start(); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.log( | ||
| "%cEnsure you are running the appropriate Fluid Server for the selected service\nUse:`npm run start:server` for local, or configure ODSP for odsp", | ||
| "font-size:30px", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this try/catch block, the call to start() below is sufficient.
Also there are some duplicated lines below that need to be cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch block and cleaned up the duplicated code sections in staging/src/start.ts. Now using simple await start();. (568ed9b)
| try { | ||
| await start(); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.log( | ||
| "%cEnsure you are running the appropriate Fluid Server for the selected service\nUse:`npm run start:server` for local, or configure ODSP for odsp", | ||
| "font-size:30px", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this try/catch block and just await start();.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch block and replaced with await start(); in task-selection/src/app.ts. (568ed9b)
| try { | ||
| await start(); | ||
| } catch (error) { | ||
| console.error(error); | ||
| console.log( | ||
| "%cEnsure you are running the appropriate Fluid Server for the selected service\nUse:`npm run start:server` for local, or configure ODSP for odsp", | ||
| "font-size:30px", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this try/catch block and just await start();.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the try/catch block and replaced with await start(); in tree-comparison/src/start.ts. (568ed9b)
| /** | ||
| * Create appropriate client based on the service type | ||
| */ | ||
| function createClient() { | ||
| const service = getSpecifiedServiceFromWebpack(); | ||
| switch (service) { | ||
| case "t9s": | ||
| return new TinyliciousClient(); | ||
| case "odsp": | ||
| // For ODSP support with the client pattern, you'll need to configure OdspClient | ||
| // with appropriate tokenProvider, siteUrl, driveId, etc. based on your setup. | ||
| // This example focuses on the TinyliciousClient pattern. | ||
| throw new Error( | ||
| "ODSP service requires additional configuration for the client pattern. " + | ||
| "Please refer to OdspClient documentation or use t9s service instead.", | ||
| ); | ||
| case "local": | ||
| throw new Error( | ||
| "Local service is not supported with the client pattern used by this example. Use t9s service instead.", | ||
| ); | ||
| default: | ||
| throw new Error(`Unsupported service: ${service}`); | ||
| } | ||
| } | ||
| // data object requires 2.41 or later. | ||
| const containerSchema = { | ||
| initialObjects: { | ||
| // Optional Presence Manager object placed within container schema for experimental presence access | ||
| // eslint-disable-next-line import/no-deprecated | ||
| presence: ExperimentalPresenceManager, | ||
| }, | ||
| } satisfies ContainerSchema; | ||
|
|
||
| export type PresenceTrackerSchema = typeof containerSchema; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't following the pattern from PR #25524, please re-attempt this change using the pattern from that PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted presence-tracker to original state and re-implemented following PR #25524 pattern: added example-driver dependency, updated webpack config, added service detection with TinyliciousClient (supports t9s service only due to client pattern limitations), and replaced start().catch() with await start();. (568ed9b)
|
@copilot please address the comments from my review |
…anges, follow PR #25524 pattern for presence-tracker Co-authored-by: ChumpChief <[email protected]>
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - client packages,Build - common-utils,Build - eslint-config-fluid,Build - eslint-plugin-fluid,Build - protocol-definitions,Build - test-tools,repo-policy-check,server-gitrest,server-gitssh,server-historian,server-routerlicious |
|
You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list. |
|
/azp run Build - api-markdown-documenter,Build - benchmark-tool,Build - build-common,Build - build-tools,Build - client packages |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Closing and reopening |
|
/azp run Build - common-utils, Build - protocol-definitions |
|
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds ODSP support to 8 out of 10 examples in the /examples/apps directory by migrating them from the TinyliciousModelLoader pattern to the unified createExampleDriver pattern established in PR #25524. The changes enable these examples to work with local, Tinylicious (t9s), and ODSP services.
Key changes include:
- Migration from service-specific loaders to the unified createExampleDriver pattern
- Addition of service-specific npm scripts (start:local, start:odsp, start:t9s)
- Updates to webpack configurations for ODSP middleware support
- Code quality improvements including TypeScript type imports and ESLint rule updates
Reviewed Changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/apps/tree-comparison/webpack.config.cjs | Added ODSP support with createExampleDriverServiceWebpackPlugin and middleware setup |
| examples/apps/tree-comparison/src/start.ts | Migrated from TinyliciousModelLoader to createExampleDriver pattern with proper service handling |
| examples/apps/tree-comparison/package.json | Added example-driver dependency and service-specific start scripts |
| examples/apps/task-selection/src/app.ts | Migrated from TinyliciousModelLoader to createExampleDriver pattern |
| examples/apps/staging/src/start.ts | Updated from lower-level tinylicious APIs to createExampleDriver pattern |
| examples/apps/data-object-grid/src/app.ts | Migrated from TinyliciousModelLoader to createExampleDriver pattern |
| examples/apps/contact-collection/src/app.ts | Migrated from TinyliciousModelLoader to createExampleDriver pattern |
| examples/apps/collaborative-textarea/src/app.ts | Migrated from TinyliciousModelLoader to createExampleDriver pattern |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| const inputHandler = (e) => { | ||
| const newValue = parseInt(e.target.value, 10); | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access | ||
| const newValue = Number.parseInt(e.target.value as string, 10); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter e should have a proper type annotation instead of relying on any and requiring an eslint disable comment. Consider using React.ChangeEvent<HTMLInputElement> for the event parameter type.
| id = container.resolvedUrl.id; | ||
| } | ||
| } else { | ||
| id = location.hash.slice(1); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is missing from the code snippet in the diff but appears to be used in the logic flow. The original code used substring(1) which was changed to slice(1) in other files but this particular line seems to be missing the update.
Add ODSP support to examples in /examples/apps
Based on PR #25524 template, implementing ODSP support for all examples under
/examples/apps.Summary:
Successfully added ODSP support to 8 out of 10 examples in
/examples/appsfollowing the same pattern established in PR #25524:✅ Fully Supported Examples (all services: local, t9s, odsp):
✅ Partially Supported Examples:
❌ Out of Scope:
Changes Applied (Following PR #25524 Pattern):
package.json Updates:
@fluid-example/example-driverdependency@fluid-example/example-webpack-integrationdevDependencystart:local- webpack serve with local servicestart:odsp- webpack serve with odsp servicestart:t9s- webpack serve with t9s servicestart- defaults to t9swebpack.config.cjs Updates:
createExampleDriverServiceWebpackPluginandcreateOdspMiddlewaresserviceparameter from env instead ofisProductionpatterncreateExampleDriverServiceWebpackPlugin(service)to pluginsApplication Code Updates:
createExampleDrivergetSpecifiedServiceFromWebpack()to determine service typeawait start();calls (no try/catch blocks)Benefits:
Usage:
This implementation provides comprehensive ODSP support across the FluidFramework examples ecosystem while maintaining backward compatibility and following established patterns.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.