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

⚗️ Use tshy instead of tsup for releasing tasks #1029

Merged
merged 10 commits into from
Nov 15, 2024
Merged

⚗️ Use tshy instead of tsup for releasing tasks #1029

merged 10 commits into from
Nov 15, 2024

Conversation

coyotte508
Copy link
Member

@coyotte508 coyotte508 commented Nov 14, 2024

Follow up to #1026

Benefit is that exports are better defined, and source-mapping works.

Also, move the generation scripts from @huggingace/tasks to @huggingface/tasks-gen cc @Wauplin @SBrandeis

So that the dev dependencies are not tacked on @huggingface/tasks. Some package managers, cough yarn cough like to download them when importing a package, even if they're irrelevant.

Comment on lines +34 to +36
"watch:cjs": "tsc --declaration --outdir dist/commonjs --module commonjs --watch",
"watch:esm": "tsc --declaration --outdir dist/esm --watch",
"watch": "npm-run-all --parallel watch:esm watch:cjs",
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @krampstudio , hopefully it still works

Comment on lines -175 to +173
const rootDir = rootDirFinder();
const rootDir = path.join(rootDirFinder(), "..", "tasks");
Copy link
Member Author

@coyotte508 coyotte508 Nov 14, 2024

Choose a reason for hiding this comment

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

We could just do const rootDir = "../tasks" since the script is always executed in the /packages/tasks-gen context

Copy link
Member

@Pierrci Pierrci left a comment

Choose a reason for hiding this comment

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

didn't try it (obviously), but lgtm 😄

packages/tasks/package.json Show resolved Hide resolved
packages/tasks/package.json Show resolved Hide resolved
packages/tasks/package.json Outdated Show resolved Hide resolved
packages/tasks/.tshy/build.json Outdated Show resolved Hide resolved
@Pierrci
Copy link
Member

Pierrci commented Nov 14, 2024

interesting CI errors haha ^^'

@coyotte508
Copy link
Member Author

interesting CI errors haha ^^'

well yea, going bakc to adding main/module/types fields

@Pierrci
Copy link
Member

Pierrci commented Nov 14, 2024

Warning: relying on top-level main/types will likely cause incorrect types to be loaded in some scenarios.

Use with extreme caution. It's almost always better to not define top-level main and types fields if you are shipping a hybrid module. Users will need to update their module and moduleResolution tsconfigs appropriately. That is a good thing, and will save them future headaches.

but yeah for now probably simpler :)

@coyotte508
Copy link
Member Author

Warning: relying on top-level main/types will likely cause incorrect types to be loaded in some scenarios.
Use with extreme caution. It's almost always better to not define top-level main and types fields if you are shipping a hybrid module. Users will need to update their module and moduleResolution tsconfigs appropriately. That is a good thing, and will save them future headaches.

but yeah for now probably simpler :)

I could remove the fields, but then we have to update the moon codebase ASAP :)

@coyotte508 coyotte508 merged commit f193bc6 into main Nov 15, 2024
5 checks passed
@coyotte508 coyotte508 deleted the tsc-only branch November 15, 2024 01:03
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