Skip to content

Deprecate containerPackageInfo parameter in createOdspCreateContainerRequest #23919

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

Merged
merged 29 commits into from
Apr 14, 2025
Merged

Deprecate containerPackageInfo parameter in createOdspCreateContainerRequest #23919

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
607654c
refactor: created overload so that we can properly deprecate the para…
MarioJGMsoft Feb 25, 2025
ef5d850
refactor: had to disable import no deprecated.
MarioJGMsoft Feb 25, 2025
ef2e8de
Merge branch 'main' into deprecateRPI
MarioJGMsoft Feb 25, 2025
7596a0c
docs: added changelist
MarioJGMsoft Mar 5, 2025
d2a4e9e
docs: updated text format
MarioJGMsoft Mar 5, 2025
f8a5b18
docs: updated comments
MarioJGMsoft Mar 5, 2025
4c771d1
docs: changed changeset doc name
MarioJGMsoft Mar 5, 2025
f47b59c
docs: updated comment
MarioJGMsoft Mar 5, 2025
93c452c
docs: updated changeset
MarioJGMsoft Mar 5, 2025
a0816bc
docs: updated changeset
MarioJGMsoft Mar 6, 2025
0016c71
docs: updated changeset based on comments
MarioJGMsoft Mar 6, 2025
19ca974
Merge remote-tracking branch 'origin' into deprecateRPI
MarioJGMsoft Mar 19, 2025
7a3f852
refactor: added fix for eslint deprecation problems
MarioJGMsoft Mar 19, 2025
66a4be2
docs: removed eslint no deprecated.
MarioJGMsoft Mar 19, 2025
adc4466
Merge branch 'microsoft:main' into deprecateRPI
MarioJGMsoft Mar 20, 2025
e3425af
Merge branch 'main' into deprecateRPI
MarioJGMsoft Mar 26, 2025
92234d7
docs: Disabled eslint deprecation
MarioJGMsoft Mar 28, 2025
70ecfc4
Merge branch 'main' into deprecateRPI
MarioJGMsoft Mar 28, 2025
065add8
Merge branch 'main' into deprecateRPI
MarioJGMsoft Apr 11, 2025
e9c3636
docs: applied changes based on comments
MarioJGMsoft Apr 11, 2025
62c3774
docs: applied changes based on comments
MarioJGMsoft Apr 11, 2025
32f5cae
docs: applied changes based on docs review
MarioJGMsoft Apr 11, 2025
a0dcc11
docs: Applied changes to changeset based on comments
MarioJGMsoft Apr 11, 2025
65124c3
docs: updated based on comments
MarioJGMsoft Apr 11, 2025
3fd97f0
applied changes based on comments.
MarioJGMsoft Apr 14, 2025
d981211
Applied changes based on comments
MarioJGMsoft Apr 14, 2025
cd8595a
Merge branch 'main' into deprecateRPI
MarioJGMsoft Apr 14, 2025
6d645ea
Merge branch 'main' into deprecateRPI
jatgarg Apr 14, 2025
d3ac29e
Merge branch 'main' into deprecateRPI
MarioJGMsoft Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions examples/utils/webpack-fluid-loader/src/odspUrlResolver.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
} from "@fluidframework/odsp-doclib-utils/internal";
import {
OdspDriverUrlResolver,
// eslint-disable-next-line import/no-deprecated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to disable import no deprecated, because even though the version that is being imported is technically not the deprecated version, eslint detected that it was using that one. Does anyone know if there's a better way to deprecate the parameter?

Copy link
Contributor

Choose a reason for hiding this comment

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

The linter doesn't understand / is being conservative. This is okay, but should add a comment with the work item that will clean this up.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only alternative might be to import the module as in import * as OdspDriver from "....
But since it is temporary, this is good enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Still looking for comment with the work item number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I recently learned of a better format to comment lint disables using --:

	// eslint-disable-next-line import/no-deprecated -- This is the non-deprecated call, but eslint doesn't distinguish. TODO AB#31049 remove suppression

createOdspCreateContainerRequest,
createOdspUrl,
} from "@fluidframework/odsp-driver/internal";
Comment on lines +15 to 18
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an example and should not be importing from /internal. Recommend fixing these when we have to touch them. See examples that use examples/.eslintrc.cjs and/or examples/.eslintrc.data.cjs.

Copy link
Member

Choose a reason for hiding this comment

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

cc @Abe27342 and @alexvy86 -- We were just talking about these in Foundation Enhancements meeting. This example which does stuff with ODSP Driver probably should be in a different group of examples that are internal-only. We don't expect anyone in the public to be doing anything with ODSP Driver.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should none of the odsp-driver exports be /legacy then? At least some of these are and should be okay to use /legacy import instead of /internal.

Copy link
Contributor

Choose a reason for hiding this comment

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

thread nudge

Copy link
Contributor

Choose a reason for hiding this comment

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

more aggressive nudge (@markfields , @Abe27342 , @alexvy86 )

Expand Down Expand Up @@ -75,6 +76,7 @@ export class OdspUrlResolver implements IUrlResolver {
this.authRequestInfo,
false,
);
// eslint-disable-next-line import/no-deprecated
return createOdspCreateContainerRequest(
`https://${this.server}`,
driveId,
Expand Down
3 changes: 3 additions & 0 deletions packages/drivers/odsp-driver/api-report/odsp-driver.legacy.alpha.api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ export function checkUrl(documentUrl: URL): DriverPreCheckInfo | undefined;
export function createLocalOdspDocumentServiceFactory(localSnapshot: Uint8Array | string): IDocumentServiceFactory;

// @alpha
export function createOdspCreateContainerRequest(siteUrl: string, driveId: string, filePath: string, fileName: string, createShareLinkType?: ISharingLinkKind): IRequest;

// @alpha @deprecated
export function createOdspCreateContainerRequest(siteUrl: string, driveId: string, filePath: string, fileName: string, createShareLinkType?: ISharingLinkKind, containerPackageInfo?: IContainerPackageInfo | undefined): IRequest;

// @alpha
Expand Down
30 changes: 30 additions & 0 deletions packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,36 @@ import { ISharingLinkKind } from "@fluidframework/odsp-driver-definitions/intern

import { buildOdspShareLinkReqParams, getContainerPackageName } from "./odspUtils.js";

/**
* Original version of createOdspCreateContainerRequest
* @legacy
* @alpha
*/
export function createOdspCreateContainerRequest(
siteUrl: string,
driveId: string,
filePath: string,
fileName: string,
createShareLinkType?: ISharingLinkKind,
): IRequest;

/**
* Overloaded version of createOdspCreateContainerRequest that takes in containerPackageInfo
* @legacy
* @alpha
* @deprecated - To be removed in 2.40
* Add containerPackageInfo to the OdspDriverUrlResolverForShareLink constructor instead; see https://github.com/microsoft/FluidFramework/issues/23882 for more details.
* Deprecating overloaded function to remove containerPackageInfo
*/
export function createOdspCreateContainerRequest(
siteUrl: string,
driveId: string,
filePath: string,
fileName: string,
createShareLinkType?: ISharingLinkKind,
containerPackageInfo?: IContainerPackageInfo | undefined,
): IRequest;

/**
* Create the request object with url and headers for creating a new file on OneDrive Sharepoint
* @param siteUrl - Base url for OneDrive
Expand Down
2 changes: 2 additions & 0 deletions packages/service-clients/odsp-client/src/odspClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
import {
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// eslint-disable-next-line import/no-deprecated
createOdspCreateContainerRequest,
createOdspUrl,
isOdspResolvedUrl,
Expand Down Expand Up @@ -209,6 +210,7 @@ export class OdspClient {
const attach = async (
odspProps?: ContainerAttachProps<OdspContainerAttachProps>,
): Promise<string> => {
// eslint-disable-next-line import/no-deprecated
const createNewRequest: IRequest = createOdspCreateContainerRequest(
connection.siteUrl,
connection.driveId,
Expand Down
2 changes: 2 additions & 0 deletions packages/test/test-drivers/src/odspDriverApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
import {
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// eslint-disable-next-line import/no-deprecated
createOdspCreateContainerRequest,
createOdspUrl,
} from "@fluidframework/odsp-driver/internal";
Comment on lines +16 to 19
Copy link
Contributor

Choose a reason for hiding this comment

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

Other option for this and other /internal import is to make the /internal export only export the non-deprecated version.
The how-to is covered at https://github.com/microsoft/FluidFramework/wiki/Change-Recipes#moving-api-to-internal

The ./internal.ts version could look like:

export const odspCreateContainerRequest: (
	siteUrl: string,
	driveId: string,
	filePath: string,
	fileName: string,
	createShareLinkType?: ISharingLinkKind,
) => ReturnType<typeof createOdspCreateContainerRequestOriginal> = createOdspCreateContainerRequestOriginal;

Since there are only two internal uses, may not be worth the effort.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's not worth the effort.

Expand All @@ -31,6 +32,7 @@ export const OdspDriverApi = {
version: pkgVersion,
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// eslint-disable-next-line import/no-deprecated
createOdspCreateContainerRequest,
createOdspUrl, // REVIEW: does this need to be back compat?
};
Expand Down
Loading