Skip to content

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 😄

@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.

3 participants