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

chore: build with rollup #35

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

Conversation

benmccann
Copy link
Contributor

The part I'm not quite as sure about is the types generation. I don't have a lot of experience doing that with rollup. It should also be noted that I don't think the existing build:types was actually used as it was outputting to the lib directory, which the package.json didn't refer to.

Please feel free to make any additional changes to clean this up. I just thought I'd send this as a start

@43081j
Copy link
Collaborator

43081j commented Aug 29, 2024

build:types was used just to run a type check (basically equivalent of --noEmit since we didn't use the output)

esbuild won't type check, but I guess the dts generation plugin tsup uses will

"default": "./dist/main.js"
},
"require": {
"types": "./dist/main.d.cts",
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should still end up with both of these (main.d.ts and main.d.cts) for typescript to be happy

what does rollup's typescript plugin output?

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'm not exactly sure how to do that with rollup's typescript plugin, but I don't think it's necessary to have both since they have identical contents

Copy link
Collaborator

Choose a reason for hiding this comment

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

It is necessary to have both. If the package type is module, typescript treats all dts as esm types and vice versa. Requiring CTS for cjs modules

So they will be the same contents but hasn't to exist twice (TS is doing the right thing here, we're all wrong for making dual packages a thing 😅)

Choose a reason for hiding this comment

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

hey guys I recently found out that "Are The Types Wrong" website have also created a CLI @arethetypeswrong/cli which allow you to test and run locally even before publishing (there's actually also the upload button on the website to upload a packed file that is also possible).

@43081j are you sure that just cloning a TypeScript d.ts file to .d.cts with same content is enough? I think that I tried that before and the CLI above didn't pass, but maybe I've done something wrong!? Side note, I tried with plain tsc, not sure if the Rollup/esbuild DTS plugins bundles dts differently though, it looks like they do compared to tsc

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, attw will also test for this

basically you should have two exports in your package.json: one for import and one for require. each has its own entrypoint and its own types if the file can't be inferred

you can see an example in tinyexec:
https://unpkg.com/browse/[email protected]/package.json

attw is happy with this, whereas if we didn't have the cts and reused the same dts, it would complain about masquerading (correctly)

Choose a reason for hiding this comment

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

but you're using tsup and yes I got it working with it, but if I use plain tsc and just copy/rename the d.ts to d.cts and update the import/require as you shown in tinyexec then it doesn't work. What I haven't tried though is to move esm/cjs into their own folders, that might avoid the masquerade detection of attw. Thanks for the info anyway

Copy link
Collaborator

Choose a reason for hiding this comment

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

tsup or not, you need two copies of the dts file (preferably a dts and a cts if the overall package is an es module)

I'm not sure why it didn't work for you. You can run tsc twice or copy the files and achieve the same result. I've done that in other packages without tsup

Overall point being you can't use one dts file for a dual package, as that would be interpreted as the root type (e.g. esm if type is module)

Copy link

@ghiscoding ghiscoding Oct 15, 2024

Choose a reason for hiding this comment

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

I figured out my problem, I wasn't using .js extensions in my imports and when testing my hybrid approach with type=module and passing it to attw it was saying that my files were the wrong format. So anyway that's fixed by using extensions and for the d.cts file, I simply added a simple Node script via clone-dts.mjs

// clone-dts.mjs
import { readFileSync, writeFileSync } from 'node:fs';

writeFileSync('dist/index.d.cts', readFileSync('dist/index.d.ts'));

isn't this the only thing missing for this PR (reverting back to the original exports)? There's probably a way to do it with a Rollup plugin, but we probably don't need to go crazy about it, my script works great with && node clone-dts.mjs in my build script. I guess this rollup-plugin-copy would also do the work

Copy link
Collaborator

Choose a reason for hiding this comment

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

you would have to copy both the source and the DTS file

js/DTS and cjs/cts (2x everything)

I've lost track of what we're discussing here too since we've gone on a tutorial-like tangent 😅

basically we need to make attw happy and we do that by having 2x everything. how you achieve that, I don't think matters much, whatever works. I don't think Ben has looked at this for a while anyway now, I'd probably have to pick it up at some point

Choose a reason for hiding this comment

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

ah sorry, I just said that I got it all working on my end and I was actually discussing this PR in my last comment. I should have been more clearer 😆

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