chore(subpaths): updated subpath creation to re-export rather than copy #8848

wise-king-sullyman · 2023-03-22T20:16:57Z

What: Resolves subpath issues encountered in #8077 and #8073

Additional issues:

patternfly-build · 2023-03-22T20:23:54Z

Preview: https://patternfly-react-pr-8848.surge.sh

A11y report: https://patternfly-react-pr-8848-a11y.surge.sh

dlabaj

Looks good just suggest not duplicating the createSubpaths.js instead pass in the subpaths using parameters or a json config file.

dlabaj · 2023-03-22T20:30:32Z

packages/react-core/scripts/createSubpaths.js

+  }
+};
+
+['next', 'deprecated', 'components', 'layouts', 'helpers'].forEach((subPathName) => {


Can you move this script into the root directory scripts folder, and then use a config file or pass in the parameters for the subpaths. This way we have it at one place instead of 2 copies. (e.g. yarn run createSubpaths.js --subpaths deperecated,components,layouts,helpers or create a subpath.json file that has the subpaths as json objects.
{ "subpaths": ["next", "deprecated", "components", "layouts", "helplers]

then in the script file just read them in by requiring the json file.

Yeah I planned on cleaning it up a bit either as part of this PR or a followup, just wanted to get something up quick so that other people could test it and actually make sure that it works before I put more time into this approach.

That works just create a follow up issue. Thanks.

dlabaj

/LGTM

jpuzz0

You have 2 issues linked in the description, but neither explicitly state what the problem is and how this solution addresses that problem.

jpuzz0 · 2023-03-23T14:58:34Z

...eact-core/src/deprecated/components/OptionsMenu/__tests__/Generated/OptionsMenuItem.test.tsx


 import { OptionsMenuItem } from '../../OptionsMenuItem';
-import { DropdownArrowContext } from '../../../../../../components/Dropdown';
+import { DropdownArrowContext } from '../../../../../components/Dropdown/';


Can you explain why this is being changed and why the import ends with a forward slash?

It was previously importing from patternfly-react/packages/react-core/components/Dropdown/index which is now no longer there. patternfly-react/packages/react-core/components would've worked, as my current approach re-exports the components at the react-core/components/index level and doesn't copy over all of the individual component directories.

The update points it to the actual source in patternfly-react/packages/react-core/src/components/Dropdown/index which seemed like a more straightforward approach.

The ending forward slash I'm not sure what the particular reason for that was. Might have just been a left over from when I was trying to figure out why the test started failing. Does ending with a slash cause an issue? I'm not opposed to removing it, just curious.

It doesn't cause an issue, but if you're adding it, there should be a reason. It implies to me that Dropdown is not the final destination for that path.

jpuzz0 · 2023-03-23T15:11:06Z

packages/react-table/scripts/createSubpaths.js

+const updatePaths = (filePath, subPathName) => {
+  const contents = readFileSync(filePath, { encoding: 'utf8' });
+  if (filePath.includes('.map')) {
+    return contents.replaceAll('../../../', `../`);


There are operations in this file that require explanation IMO. For instance, with this particular condition for .map filePaths, why do we need to do this?

The sourcemap files point towards the source rather than pointing to dist like the exporting index files do, this conditional makes sure that we apply the proper update to both file types.

Also just generally I know a lot of the logic here isn't optimal. As I mentioned with Don's comment I plan to refine how exactly this is done, but wanted to validate that the fundamental change itself is good before investing more time in doing so.

What is happening and why isn't clear to me, so if I am to ignore the readability of this and expect the logic to change, validity at this point for me would be proof that this accomplishes what we want, and I'll pose that question to you. Meaning, have you tested this out with other packages locally?

In my local testing existing imports from /deprecated and /next seem to still work properly (as can be seen in the OptionsMenu and Select next docs pages in this PRs preview)

Without this change the deprecated wizard with drawer example is broken when trying to import from just /deprecated, as demonstrated below:
https://user-images.githubusercontent.com/7115053/227536041-57bb9c78-8254-412a-84aa-97d245df1b8a.mov

And with this change:
https://user-images.githubusercontent.com/7115053/227539676-a7df08f2-44da-4f11-9df5-f1ed47e8bb23.mov

Based on this testing I think everything is good to go functionality wise, but since I don't think I fully understand the reasoning behind the approach you went with previously I figured it would be best to double check with you.

I went ahead and tested this out against a sample project to make sure /deprecated and /next components can still be imported from @patternfly/react-core/next and @patternfly/react-core/deprecated (which was the point of the change in the first place). Just testing within this repo is less reliable IMO, but certainly the documentation working as expected is a good sign.

jpuzz0 · 2023-03-23T15:12:36Z

packages/react-table/scripts/createSubpaths.js

@@ -0,0 +1,32 @@
+/**
+ * Re-export subpath modules into the package root directory for ease of access.


I don't think we can keep this description so simple anymore since there is a lot more going on than a single copySync.

Do you have any recommendations?

It's doing a lot more operations, but it's still essentially completing the same goal of making the components available via importing from @patternfly/react-core/deprecated etc, it just does it via re-exporting rather than copying the actual component files.

I don't, and maybe it's just that we need better comments around the operations that are taking place below.

wise-king-sullyman · 2023-03-23T16:18:06Z

@jpuzz0 regarding what this fix is actually meant to address, I've created #8853 with some additional context.

jpuzz0 · 2023-03-23T21:42:17Z

@wise-king-sullyman My feedback are mainly around comments and documentation of the solution, so if there's proof that this solution works and we need this to unblock others and we have an issue to cleanup later, I will approve.

jpuzz0

LGTM, but I do wonder why not take the time to cleanup the script now. I'm not sure how urgent this is though. Introducing new code with the expectation of it changing soon after merging isn't ideal.

wise-king-sullyman · 2023-03-24T15:09:17Z

Yeah agree that it isn't ideal, reasons for urgency are that today is PF code freeze day and we've got a few deprecation PRs in this sprint that will either need this fix to be in or will require consumers to import from dist/esm/deprecated to get bug free versions of some of them.

patternfly-build · 2023-03-24T15:48:21Z

Your changes have been released in:

@patternfly/react-code-editor@5.0.0-alpha.46
@patternfly/react-core@5.0.0-alpha.46
@patternfly/react-docs@6.0.0-alpha.49
demo-app-ts@5.0.0-alpha.30
@patternfly/react-table@5.0.0-alpha.46

Thanks for your contribution! 🎉

chore(subpaths): updated subpath creation to re-export rather than copy

aafe17a

chore(OptionsMenu): updated import path in test

28076f7

dlabaj requested changes Mar 22, 2023

View reviewed changes

dlabaj approved these changes Mar 22, 2023

View reviewed changes

jpuzz0 self-requested a review March 23, 2023 15:13

jpuzz0 reviewed Mar 23, 2023

View reviewed changes

chore(OptionsMenu): removed unneeded ending slash in import

3f78b42

wise-king-sullyman mentioned this pull request Mar 24, 2023

Cleanup subpath creation script #8862

Closed

wise-king-sullyman requested a review from jpuzz0 March 24, 2023 14:41

jpuzz0 approved these changes Mar 24, 2023

View reviewed changes

tlabaj merged commit d1d7ec6 into patternfly:v5 Mar 24, 2023

wise-king-sullyman deleted the re-export-modules-for-subpaths branch March 24, 2023 15:32

		@@ -0,0 +1,32 @@
		/**
		* Re-export subpath modules into the package root directory for ease of access.

chore(subpaths): updated subpath creation to re-export rather than copy #8848

chore(subpaths): updated subpath creation to re-export rather than copy #8848

Uh oh!

Conversation

wise-king-sullyman commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patternfly-build commented Mar 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlabaj left a comment

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!

dlabaj left a comment

Choose a reason for hiding this comment

Uh oh!

jpuzz0 left a comment

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!

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!

wise-king-sullyman commented Mar 23, 2023

Uh oh!

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

Uh oh!

Uh oh!

jpuzz0 left a comment

Choose a reason for hiding this comment

Uh oh!

wise-king-sullyman commented Mar 24, 2023

Uh oh!

patternfly-build commented Mar 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wise-king-sullyman commented Mar 22, 2023 •

edited

Loading

patternfly-build commented Mar 22, 2023 •

edited

Loading

jpuzz0 commented Mar 23, 2023 •

edited

Loading