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 all 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
12 changes: 12 additions & 0 deletions .changeset/wicked-eagles-search.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
"@fluidframework/odsp-driver": minor
"__section": deprecation
---

The containerPackageInfo parameter in createOdspCreateContainerRequest() is now deprecated

The `containerPackageInfo` parameter in `createOdspCreateContainerRequest()` is now deprecated and will be removed in version 2.40.0.

The name of the containerPackage can no longer be sent through the request. Instead, it can be added in the constructor of `OdspDriverUrlResolverForShareLink`.

Check failure on line 10 in .changeset/wicked-eagles-search.md

View workflow job for this annotation

GitHub Actions / vale 

[vale] reported by reviewdog 🐶
[Vale.Spelling] Did you really mean 'containerPackage'?

Raw Output:
{"message": "[Vale.Spelling] Did you really mean 'containerPackage'?", "location": {"path": ".changeset/wicked-eagles-search.md", "range": {"start": {"line": 10, "column": 17}}}, "severity": "ERROR"}

See [issue #23882](https://github.com/microsoft/FluidFramework/issues/23882) for more details.
4 changes: 4 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,8 @@ import {
} from "@fluidframework/odsp-doclib-utils/internal";
import {
OdspDriverUrlResolver,
// The comment will be removed up when the deprecated code is removed in AB#31049
// 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 +77,8 @@ export class OdspUrlResolver implements IUrlResolver {
this.authRequestInfo,
false,
);
// The comment will be removed up when the deprecated code is removed in AB#31049
// eslint-disable-next-line import/no-deprecated
return createOdspCreateContainerRequest(
`https://${this.server}`,
driveId,
Expand Down
5 changes: 4 additions & 1 deletion 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,7 +11,10 @@ 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, containerPackageInfo?: IContainerPackageInfo | undefined): IRequest;
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 | undefined, containerPackageInfo: IContainerPackageInfo | undefined): IRequest;

// @alpha
export function createOdspUrl(l: OdspFluidDataStoreLocator): string;
Expand Down
46 changes: 45 additions & 1 deletion packages/drivers/odsp-driver/src/createOdspCreateContainerRequest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,51 @@ import { buildOdspShareLinkReqParams, getContainerPackageName } from "./odspUtil
* @param fileName - name of the new file to be created
* @param createShareLinkType - type of sharing link you would like to create for this file. ShareLinkTypes
* will be deprecated soon, so for any new implementation please provide createShareLinkType of type ShareLink
* @param containerPackageInfo - container package information which will be used to extract the container package name.
* @legacy
* @alpha
*/
export function createOdspCreateContainerRequest(
siteUrl: string,
driveId: string,
filePath: string,
fileName: string,
createShareLinkType?: ISharingLinkKind,
): IRequest;

/**
* Create the request object with url and headers for creating a new file on OneDrive Sharepoint
* @param siteUrl - Base url for OneDrive
* @param driveId - drive identifier
* @param filePath - path where file needs to be created
* @param fileName - name of the new file to be created
* @param createShareLinkType - type of sharing link you would like to create for this file. ShareLinkTypes
* will be deprecated soon, so for any new implementation please provide createShareLinkType of type ShareLink
* @param containerPackageInfo - **Deprecated Parameter** - container package information which will be used to extract the container package name.
* If not given that means that the container package does not have a name.
* @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 | undefined,
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
* @param driveId - drive identifier
* @param filePath - path where file needs to be created
* @param fileName - name of the new file to be created
* @param createShareLinkType - type of sharing link you would like to create for this file. ShareLinkTypes
* will be deprecated soon, so for any new implementation please provide createShareLinkType of type ShareLink
* @param containerPackageInfo - **Deprecated Parameter** - container package information which will be used to extract the container package name.
* If not given that means that the container package does not have a name.
* @legacy
* @alpha
Expand Down
4 changes: 4 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,8 @@ import {
import {
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// The comment will be removed up when the deprecated code is removed in AB#31049
// eslint-disable-next-line import/no-deprecated
createOdspCreateContainerRequest,
createOdspUrl,
isOdspResolvedUrl,
Expand Down Expand Up @@ -209,6 +211,8 @@ export class OdspClient {
const attach = async (
odspProps?: ContainerAttachProps<OdspContainerAttachProps>,
): Promise<string> => {
// The comment will be removed up when the deprecated code is removed in AB#31049
// eslint-disable-next-line import/no-deprecated
const createNewRequest: IRequest = createOdspCreateContainerRequest(
connection.siteUrl,
connection.driveId,
Expand Down
4 changes: 4 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,8 @@ import {
import {
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// The comment will be removed up when the deprecated code is removed in AB#31049
// 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 +33,8 @@ export const OdspDriverApi = {
version: pkgVersion,
OdspDocumentServiceFactory,
OdspDriverUrlResolver,
// The comment will be removed up when the deprecated code is removed in AB#31049
// eslint-disable-next-line import/no-deprecated
createOdspCreateContainerRequest,
createOdspUrl, // REVIEW: does this need to be back compat?
};
Expand Down
Loading