feat: add deprecated subpaths by nicolethoen · Pull Request #3291 · patternfly/patternfly-org

nicolethoen · 2022-11-14T20:54:57Z

patternfly-build · 2022-11-14T21:03:14Z

PF4 preview: https://patternfly-org-pr-3291-v4.surge.sh/v4

jpuzz0 · 2022-11-14T21:44:38Z

    ...reactTableModule,
-    ...(source === 'react-next' ? reactCoreNextModule : {})
+    ...(source === 'react-next' ? reactCoreNextModule : {}),
+    ...(source === 'react-deprecated' ? reactCoreDeprecatedModule : {})


It doesn't look like reactCoreDeprecatedModule is defined, and I think we want to extend upon react-table's module for this scope and not react-core (I assume we'll want both eventually, but for this change, at least react-table).

jpuzz0 · 2022-11-15T19:43:32Z

  'react-composable': 1.2,
-  'react-legacy': 1.3,
+  'react-deprecated': 1.3,
+  'react-legacy': 1.4,


Do we still have plans to use react-legacy after this change and introduction of react-deprecated?

ultimately react-deprecated will replace the legacy here so I can remove it, but probably not until it's been removed after the breaking change. I don't want to remove it before then.
The deprecated work is going in the v5 branch

Got it, so maybe we make a follow-up issue to remove this when we're ready?

jpuzz0 · 2022-11-15T19:45:24Z

@@ -281,9 +281,14 @@ function writeIndex() {
 }

 // Build unique names for components if they are a part of the "next" module.


If we keep this function as you've expanded upon it, we should update this comment.

jpuzz0 · 2022-11-15T19:59:50Z

-function getTsDocName(name, isNextComponent) {
-  return `${name}${isNextComponent ? '-next' : ''}`;
-};
+function getTsDocName(name, isNextComponent, isDeprecatedComponent) {


So as to not have to add new parameters to this function whenever a new condition arises, we might want to consider reworking this a bit. This is what I had in mind roughly, where if there's another "variant" at any point, we just have to update the 1 location in getTsDocNameVariant:

// Build unique names for components with a "variant" extension function getTsDocName(name, variant) { return `${name}${variant ? `-${variant}` : ''}`; } function getTsDocNameVariant(source) { if (source.includes('next')) { return 'next'; } if (source.includes('deprecated')) { return 'deprecated'; } }

and the 2 usages above would then look like the following:
tsDocs[getTsDocName(name, getTsDocNameVariant(file))] = { name, description, props };
and
const name = getTsDocName(componentName, getTsDocNameVariant(source));

jpuzz0

Looks good. Once Jenny's PR is merged; patternfly/patternfly-react#8381, this build should pass.

jpuzz0 · 2023-01-19T15:24:38Z

  // React-table MD
-  sourceMD(path.join(reactTablePath, '/**/TableComposable/examples/*.md'), 'react-composable');
-  sourceMD(path.join(reactTablePath, '/**/demos/*.md'), 'react-demos');
+  sourceMD(path.join(reactTablePath, '/**/TableComposable/examples/*.md'), 'react');


Was this tested locally by any chance? This regex might conflict with the deprecated path regex on line 61. To prevent the potential for that we can remove the wildcard at the beginning of this line and replace it with components (e.g. /components/TableComposable/...).

not recently but i'll do that

@jeffpuzzo so i'm realizing now that this line will be better tested in the react-docs package where we have our own patternfly-docs.source.js file defined. patternfly-react does not consume org's v4 package, it's actually the other way around, so if these lines dont work in the react workspace when the change is made there, then we can update it here to match whatever worked in the workspace. If this is good to be merged, hopefully this will unblock the work to deprecate table

Ah right, that makes sense.

jpuzz0 · 2023-01-30T19:12:08Z

-        '@patternfly/react-core/next': '@patternfly/react-core/dist/esm/next/index.js' // Can remove when webpack is updated to v5
+        '@patternfly/react-core/next': '@patternfly/react-core/dist/esm/next/index.js', // Can remove when webpack is updated to v5
+        '@patternfly/react-core/deprecated': '@patternfly/react-core/dist/esm/deprecated/index.js', // Can remove when webpack is updated to v5
+        '@patternfly/react-table/deprecated': '@patternfly/react-table/dist/esm/deprecated/index.js' // Can remove when webpack is updated to v5


I think all of these path aliases can be removed now since we're no longer using package exports in react-core/table.

jpuzz0 · 2023-01-30T19:12:52Z

  sourceProps(path.join(reactCodeEditorPath, '/**/*.tsx'),reactPropsIgnore);
  sourceProps(path.join(reactChartsPath, '/**/*.tsx'),reactPropsIgnore);
  sourceProps(path.join(reactLogViewerPath, '/**/*.tsx'), reactPropsIgnore);
+  //sourceProps(path.join(reactDragDropPath, '/**/*.tsx'), reactPropsIgnore);


Maybe for now, unless we want to add TODO comments explaining when we'd add this back in, we can remove this commented out line along with lines 43-44 & 81?

jpuzz0 · 2023-01-30T19:58:50Z

-    ...(source === 'react-next' ? reactCoreNextModule : {})
+    ...(source === 'react-next' ? reactCoreNextModule : {}),
+    ...(source === 'react-deprecated' ? reactCoreDeprecatedModule : {}),
+    ...(source === 'react-deprecated' ? reactTableDeprecatedModule : {})


I think we can use the condition source === 'react-deprecated' just once for the inclusion of both of these deprecated modules.

jpuzz0

Tested this locally against patternfly-react successfully. Code changes LGTM.

jenny-s51

Looks great @nicolethoen !! 🚀

Also tested locally and I am able to now add demos under Table -> react-deprecated .

nicolethoen requested review from jenny-s51 and jpuzz0 November 14, 2022 20:54

nicolethoen mentioned this pull request Nov 14, 2022

Create "deprecated" subpaths in react-table patternfly/patternfly-react#8362

Closed

jpuzz0 reviewed Nov 14, 2022

View reviewed changes

jpuzz0 reviewed Nov 15, 2022

View reviewed changes

jpuzz0 previously approved these changes Nov 17, 2022

View reviewed changes

jenny-s51 mentioned this pull request Dec 1, 2022

Clean up comments in patternfly-docs.source.js patternfly/patternfly-react#8401

Closed

nicolethoen dismissed jpuzz0’s stale review via f5a5722 January 18, 2023 22:28

nicolethoen force-pushed the deprecated_subpaths branch from 3cc0849 to f5a5722 Compare January 18, 2023 22:28

nicolethoen requested a review from jpuzz0 January 18, 2023 22:31

jpuzz0 reviewed Jan 19, 2023

View reviewed changes

nicolethoen added 4 commits January 27, 2023 16:03

feat: add deprecated subpaths

cf6824f

make sure paths reference react-table

81c962e

update per PR comments

3533764

adding deprecated paths for react-core

dcc4165

nicolethoen force-pushed the deprecated_subpaths branch from f5a5722 to dcc4165 Compare January 27, 2023 21:41

jpuzz0 reviewed Jan 30, 2023

View reviewed changes

clean up

1cbf4d2

jpuzz0 approved these changes Jan 30, 2023

View reviewed changes

jenny-s51 approved these changes Jan 31, 2023

View reviewed changes

nicolethoen merged commit b4324a7 into patternfly:main Feb 2, 2023

		@@ -281,9 +281,14 @@ function writeIndex() {
		}

		// Build unique names for components if they are a part of the "next" module.

Conversation

nicolethoen commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Nov 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 left a comment • edited by jenny-s51 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpuzz0 left a comment

Choose a reason for hiding this comment

Uh oh!

jenny-s51 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nicolethoen commented Nov 14, 2022 •

edited

Loading

patternfly-build commented Nov 14, 2022 •

edited

Loading

jpuzz0 left a comment •

edited by jenny-s51

Loading

jpuzz0 Jan 30, 2023 •

edited

Loading