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.

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

fix: move package to ESM-Only and fix package exports #223

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

userquin
Copy link
Contributor

@userquin userquin commented Jan 18, 2024

I cannot build date-picker-input on my local, once the corresponding dts generated, the subpackage types should be fixed: dist/date-picker-input/date-picker-input.d.ts missing.

Lit is using ESM-Only, you can check latest and 2.8.0 versions (v2.8.0 being used in 6 rc33):

Check screenshot in this comment: #222 (comment)

pnpm build && pnpm pack, go to https://arethetypeswrong.github.io/ and upload the tgz file: refresh page between builds.

EDIT: I can also include "packageManager": "pnpm@8.14.1" in the package.json

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

Attention: 31 lines in your changes are missing coverage. Please review.

Comparison is base (3582b2c) 100.00% compared to head (e504a68) 92.82%.
Report is 8 commits behind head on main.

Files Patch % Lines
src/date-picker-input/date-picker-input.ts 82.50% 7 Missing and 7 partials ⚠️
src/year-grid/year-grid.ts 57.14% 9 Missing and 3 partials ⚠️
src/month-calendar/month-calendar.ts 90.00% 1 Missing and 3 partials ⚠️
src/date-picker/date-picker.ts 98.78% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #223      +/-   ##
===========================================
- Coverage   100.00%   92.82%   -7.18%     
===========================================
  Files           58       58              
  Lines         3305      711    -2594     
  Branches       238      155      -83     
===========================================
- Hits          3305      660    -2645     
- Misses           0       32      +32     
- Partials         0       19      +19     
Flag Coverage Δ
unit_tests 92.82% <88.92%> (-7.18%) ⬇️
unit_tests_helpers ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -104,6 +92,43 @@
"main": "./dist/date-picker/app-date-picker.js",
"module": "./dist/date-picker/app-date-picker.js",
"typings": "./dist/date-picker/app-date-picker.d.ts",
"typesVersions": {
Copy link
Owner

Choose a reason for hiding this comment

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

@userquin Could you please explain why is this required?

IIUC, exports is more than enough for TypeScript to read the corresponding types correctly from this article.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If your tsconfig file has compilerOptions.moduleResolution="node" TS will be using node10, there shoud be dts files in root folder and this package doesn't include them, types version entry will allow TS to detect dts files.

Copy link
Owner

@motss motss Jan 19, 2024

Choose a reason for hiding this comment

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

TS will be using node10, there shoud be dts files in root folder and this package doesn't include them, types version entry will allow TS to detect dts files.

I don't remember that's how TypeScript works. and TypeScript does not enforce us to have a root dts file. CMIIW.

IIRC, it reads from types or typings in the package.json and you can see that I already it defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's when importing app-datepicker, the problem when importing any app-datepicker subpackage.

Copy link
Owner

Choose a reason for hiding this comment

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

@userquin I looked into this and I need your help to clarify some things.

import { AppDatePicker } from 'app-datepicker/app-date-picker';

This import statement uses the exports field in package.json, which is only supported by node16 and later versions. To make this work in node10, we can use the typesVersions field to ship different types for different TypeScript versions.

IMHO, this does not feel right to me to use typesVersions for such use case because node10 users should avoid this kind of import and instead use relative paths to point to a specific file, like this:

// instead of doing this
// import { AppDatePicker } from 'app-datepicker/app-date-picker';

// node10 users should do this
import { AppDatePicker } from 'app-datepicker/dist/date-picker/app-date-picker.js';

WDYT?

Copy link
Contributor Author

@userquin userquin Jan 21, 2024

Choose a reason for hiding this comment

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

You need support for node, node16(esm) and bundler.
The problem is in d.ts files, we will need to generate d.mts files for node16: when using relative imports inside a d.ts you need the extension and so you cannot reuse the same d.ts for node, node16 and bundler.
I'm going to try to use tsup and unbuild to generate proper d.ts and d.mts files.
In node16 you need to use .js extensions inside the d.ts (m.dts) when importing local modules (relative path).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check this huge PR migrating to ESM-Only UnoCSS packages:
unocss/unocss#3380

Copy link
Owner

Choose a reason for hiding this comment

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

I kind of get it but this project does not support CommonJS/ require at all. It aims to ship only ESM files because it is supposed to run only on the browsers at the end of the day. Generating types just to support node10 and CommonJS is definitely out of the scope of the project unless it is a typing bug/ issue. TypeScript using moduleResolution=node can still reference the correct file and typing file without any issue as long as they use the relative file path. moduleResolution=node16|bundler and ESM are what this project supports and anything other than that is no-no. Hope it makes sense.

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 didn't mention CJS at all ;). If you only support node16 and bundler you only need to fix typings.ts modules in this PR, and we can also remove types versions (node 10) from package.json file.

"default": "./dist/root-element/root-element.js"
},
"./year-grid": {
"types": "./dist/year-grid/year-grid.d.ts",
"import": "./dist/year-grid/year-grid.js",
Copy link
Owner

Choose a reason for hiding this comment

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

IIUC, import is redundant as this package only supports ESM and default can do what we want it to do but I chose to keep it back then. I gues it's safe to remove those. Thanks for the removal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is about moving to ESM only, it is equivalent (you have type module)

@userquin
Copy link
Contributor Author

userquin commented Jan 19, 2024

You can use a bundler instead using tsc, for example you can use rollup, tsup or unbuild, this way you can write the build in the corresponding folders.

As an example, you can check lit (using rollup) package, the corresponding dts files in the root folder and subpackage exports with the corresponding folders (index.dts, decorators.d.ts, async-directive.d.ts, directives.d.ts... in root folder, decorators folder with the corresponding dts inside. This way when using Node/Node10, Node 16 or Bundler TS will not complain when resolving types:

imagen

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.

None yet

2 participants