chore(subpaths): updated subpath creation to re-export rather than copy #8848
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /** | ||
| * Re-export subpath modules into the package root directory for ease of access. | ||
| */ | ||
| const { readdirSync, readFileSync, writeFileSync, mkdirSync } = require('fs'); | ||
| const { resolve, join } = require('path'); | ||
|
|
||
| const isIndex = (fileName) => fileName.match(/^index\./); | ||
|
|
||
| const updatePaths = (filePath, subPathName) => { | ||
| const contents = readFileSync(filePath, { encoding: 'utf8' }); | ||
| if (filePath.includes('.map')) { | ||
| return contents.replaceAll('../../../', `../`); | ||
| } else { | ||
| return contents.replaceAll('./', `../dist/esm/${subPathName}/`); | ||
| } | ||
| }; | ||
|
|
||
| ['next', 'deprecated', 'components', 'layouts', 'helpers'].forEach((subPathName) => { | ||
| const distPath = resolve(__dirname, `../dist/esm/${subPathName}`); | ||
| const distFileNames = readdirSync(distPath); | ||
| const distIndexFileNames = distFileNames.filter((fileName) => isIndex(fileName)); | ||
|
|
||
| const destinationDir = resolve(__dirname, `../${subPathName}`); | ||
| mkdirSync(destinationDir, { recursive: true }); | ||
|
|
||
| distIndexFileNames.forEach((fileName) => { | ||
| const filePath = join(__dirname, '../dist/esm', subPathName, fileName); | ||
| const newFileContent = updatePaths(filePath, subPathName); | ||
| const fileDestination = join(destinationDir, fileName); | ||
| writeFileSync(fileDestination, newFileContent); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /** | ||
| * Re-export subpath modules into the package root directory for ease of access. | ||
|
Contributor
There was a problem hiding this comment. I don't think we can keep this description so simple anymore since there is a lot more going on than a single copySync.
Contributor
Author
There was a problem hiding this comment. 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
Contributor
There was a problem hiding this comment. I don't, and maybe it's just that we need better comments around the operations that are taking place below. |
||
| */ | ||
| const { readdirSync, readFileSync, writeFileSync, mkdirSync } = require('fs'); | ||
| const { resolve, join } = require('path'); | ||
|
|
||
| const isIndex = (fileName) => fileName.match(/^index\./); | ||
|
|
||
| const updatePaths = (filePath, subPathName) => { | ||
| const contents = readFileSync(filePath, { encoding: 'utf8' }); | ||
| if (filePath.includes('.map')) { | ||
| return contents.replaceAll('../../../', `../`); | ||
|
Contributor
There was a problem hiding this comment. There are operations in this file that require explanation IMO. For instance, with this particular condition for
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. 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?
Contributor
Author
There was a problem hiding this comment. 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: And with this change: 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.
Contributor
There was a problem hiding this comment. I went ahead and tested this out against a sample project to make sure /deprecated and /next components can still be imported from |
||
| } else { | ||
| return contents.replaceAll('./', `../dist/esm/${subPathName}/`); | ||
| } | ||
| }; | ||
|
|
||
| ['deprecated', 'components'].forEach((subPathName) => { | ||
| const distPath = resolve(__dirname, `../dist/esm/${subPathName}`); | ||
| const distFileNames = readdirSync(distPath); | ||
| const distIndexFileNames = distFileNames.filter((fileName) => isIndex(fileName)); | ||
|
|
||
| const destinationDir = resolve(__dirname, `../${subPathName}`); | ||
| mkdirSync(destinationDir, { recursive: true }); | ||
|
|
||
| distIndexFileNames.forEach((fileName) => { | ||
| const filePath = join(__dirname, '../dist/esm', subPathName, fileName); | ||
| const newFileContent = updatePaths(filePath, subPathName); | ||
| const fileDestination = join(destinationDir, fileName); | ||
| writeFileSync(fileDestination, newFileContent); | ||
| }); | ||
| }); | ||
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.
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,helpersor 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.
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.
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.
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.
That works just create a follow up issue. Thanks.