Skip to content
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

Jump to bottom

Fix tree shaking #21

Merged
merged 18 commits into from
Oct 19, 2023
Merged

Fix tree shaking #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1611527
New export
cdedreuille Oct 16, 2023
1655f06
Test with new export
cdedreuille Oct 16, 2023
5fc08fe
Update tsup.config.ts
cdedreuille Oct 16, 2023
30271e7
Update tsup.config.ts
cdedreuille Oct 16, 2023
da9d4e1
Test new structure
cdedreuille Oct 16, 2023
f6d04c9
Test
cdedreuille Oct 16, 2023
a240a20
Update package.json
cdedreuille Oct 16, 2023
4682571
Update package.json
cdedreuille Oct 16, 2023
c76c8d6
Try something
cdedreuille Oct 17, 2023
41fcadf
Update tsup.config.ts
cdedreuille Oct 17, 2023
1d05674
Test
cdedreuille Oct 17, 2023
b9bd056
Stuff
cdedreuille Oct 17, 2023
3072114
Cleaning
cdedreuille Oct 17, 2023
acbea1d
Update package.json
cdedreuille Oct 17, 2023
2230fe1
Update tsup.config.ts
cdedreuille Oct 18, 2023
f9be3b6
Change name of all icons
cdedreuille Oct 18, 2023
5fc3960
Update package.json
cdedreuille Oct 18, 2023
bf34a28
Update package.json
cdedreuille Oct 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
8 changes: 5 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,17 @@
"chromatic": "npx chromatic --project-token=chpt_d0c5927e55681dd",
"generate-icons": "ts-node ./src/generate.ts"
},
"sideEffect": false,
"types": "./dist/index.d.ts",
"sideEffects": false,
"main": "./dist/index.js",
"module": "./dist/index.mjs",
"exports": {
".": {
"require": "./dist/index.js",
Copy link

Choose a reason for hiding this comment

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

Does that mean that we don't provide cjs exports anymore?

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 found a couple of places where it says that setting "main": "./dist/index.js", is enough for cjs. But I might be wrong. What would be your recommendation?

Copy link

Choose a reason for hiding this comment

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

I am not that experienced with this kind of setting. @ndelangen is the best person to ask.

Copy link
Member

Choose a reason for hiding this comment

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

On what basis are we making these changes? What's the rationale? Do we know what effects these changes will have and why?

I'd expect a PR description detailing what the problem was, how the code change fixes it and potentially the alternatives and potential alternatives.

This is the first time for me looking at this codebase as far as I can remember, so I can't determine if this change is ok or now.

  • removing cjs exports seems/feels wrong
  • adding the "pure"-comments in source seems wrong too.
    But maybe there are valid reasons for doing this. I do not have enough context.

I do not trust the screenshots of a website I do not know the details of.
I was told it's running esbuild, but it's extremely dependent on how esbuil is configured, as well as other factors in our toolchain if the code is actually treeshaken in the end or not.
I'd like to see the actual tree-shaken code when consumed in the manager itself, and inside a manager-entry.

Copy link
Contributor Author

@cdedreuille cdedreuille Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@ndelangen We are not using the library anywhere right now. The question is more about is the best way to export the library in package.json. I made this change in package.json because it was recommended in a couple of articles I found but I'm definitely not an expert in that field.

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 guess I'm not sure what's the difference between:

"main": "./dist/index.js",
"module": "./dist/index.mjs",

and

"main": "./dist/index.js",
"exports": {
  ".": {
    "require": "./dist/index.js",
    "import": "./dist/index.mjs"
  }
}

In the later, it seem we still need "main". I can't find a clear answer on the web.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By the way @ndelangen I brought back the change.

Copy link
Member

@ndelangen ndelangen Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Adding the pure comment is a compromise we found with @JReinhold and @valentinpalkovic to make sure forward Ref is considered pure. This PR for more context: evanw/esbuild#2749

Thank you for that context & link, I agree that it makes sense then.

brought back the change

Ok, I see. I'm fine with removing it, if we're sure that's the right thing to do, and we can argue why, and preferable demonstrate.

package.json exports fields are complex, because they are used by a variety of tools in a variety of ways depending on multiple variables

"import": "./dist/index.mjs"
"import": "./dist/index.mjs",
"types": "./dist/index.d.ts"
}
},
"types": "./dist/index.d.ts",
"files": [
"dist"
],
Expand Down
6 changes: 3 additions & 3 deletions src/generate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@ export const getGroups = async (figmaFileId: string) => {
console.log(chalk.cyanBright('-> Converting to React components'));
downloadedSVGsData.forEach((svg) => {
const svgCode = svg.data;
const componentName = toPascalCase(svg.name);
const componentName = `${toPascalCase(svg.name)}Icon`;
const componentFileName = `${componentName}.tsx`;
const storyFileName = `${componentName}.stories.tsx`;
const storyFileName = `${componentName}.stories.ts`;

// Converts SVG code into React code using SVGR library
const componentCode = svgr.sync(svgCode, svgrConfig, {
Expand Down Expand Up @@ -114,7 +114,7 @@ export const getGroups = async (figmaFileId: string) => {
})
);

// 8. Generate icons.ts
// 8. Generate index.ts
console.log(chalk.yellowBright('-> Generating icons file'));
createIndex({ downloadedSVGsData, groupsWithComponents });

Expand Down
Loading
Loading