Skip to content

Fix package structure and use shared Prettier, ESLint, TypeScript configs - #13

Merged
devgioele merged 11 commits into
mainfrom
fix-package-structure
Jul 31, 2023
Merged

Fix package structure and use shared Prettier, ESLint, TypeScript configs#13
devgioele merged 11 commits into
mainfrom
fix-package-structure

Conversation

@devgioele

@devgioele devgioele commented Jul 28, 2023

Copy link
Copy Markdown

Because we intend to support Node.js versions <= 12, we have to renounce on the multiple entry points. Otherwise we cannot use the package.

We now also use the shared configs for Prettier, ESLint and TypeScript.

@devgioele devgioele self-assigned this Jul 28, 2023
@devgioele
devgioele requested a review from mmalfertheiner July 28, 2023 12:13
@devgioele
devgioele changed the base branch from main to use-shared-prettier-eslint July 28, 2023 12:13
@devgioele
devgioele changed the base branch from use-shared-prettier-eslint to main July 28, 2023 12:14
@devgioele devgioele changed the title Fix package structure Fix package structure and use shared Prettier, ESLint, TypeScript configs Jul 28, 2023
Comment thread tsconfig.esm.json
@@ -3,17 +3,12 @@
"compilerOptions": {
"module": "es2020",
"outDir": "./dist/esm",

@mmalfertheiner mmalfertheiner Jul 28, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In the old we had the output on ./dist for the esm module.

That allows you to import import { Query } from '@aboutbits/react-pagination/dist/engine'.

I'm not sure if the current version would work because you tell them to always look into /dist/esm

Moreover I find it a bit strange that you have esm in the import, because it looks like the programmer has to explicitly select the component with esm module resolution.

@devgioele devgioele Jul 28, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I chose this output directory so that the user of the library knows what is being used. Because we have no entry point, the user must choose. I think that naming the directories dist/esm and dist/cjs is more clear than naming them dist and dist/cjs or dist/esm and dist.
Do you think it is better to default to esm or cjs? If yes, to which one would you default?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see it more from the user perspective and there you don't care about the module system. You just want to import the component. Right now a user would have to decide. Should I take esm or cjs?

I would therefore choose a default. I think we use esm in the other packages.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

In the old package @aboutbits/react-pagination and the package @aboutbits/react-toolbox we use cjs as the default. What default should I take?

@devgioele devgioele Jul 30, 2023

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see that a build of Next.js fails when importing the esm version. So should we default to cjs?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for checking. Go ahead with `cjs.

@devgioele
devgioele requested a review from mmalfertheiner July 28, 2023 12:53
@devgioele
devgioele merged commit 8af29dc into main Jul 31, 2023
@devgioele
devgioele deleted the fix-package-structure branch July 31, 2023 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants