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

Provide types for cjs #39

Closed
asamuzaK opened this issue Jan 12, 2025 · 7 comments · Fixed by #41
Closed

Provide types for cjs #39

asamuzaK opened this issue Jan 12, 2025 · 7 comments · Fixed by #41

Comments

@asamuzaK
Copy link
Owner

asamuzaK commented Jan 12, 2025

Split from #38

Maybe tsup should do?

@crutchcorn
Copy link
Contributor

Heads up - we've ran into many headaches with tsup generating incorrect TypeScript files with TanStack Packages; which is one of the reasons we moved to our Vite-based Config package

@asamuzaK
Copy link
Owner Author

Oh, thanks for the info.
I would like to know the details, if you have any records / logs, could you please show them to me?

https://github.com/asamuzaK/cssColor/pull/40/files#diff-49d1dc69c8e7ce7933ae32b3bfadd131053a95f01f43e69886688f576f4d93e6
At a glance, there doesn't seem to be any particular problem though.
Hmm...

Anyway, I'm not planning to merge it unless we can't get confirmation from everyone else.
I just want to merge #37 as soon as possible...

@crutchcorn
Copy link
Contributor

This is the main issue we ran into:

egoist/tsup#993

@asamuzaK
Copy link
Owner Author

asamuzaK commented Jan 12, 2025

Read below:

It seems that this problem itself is fixed, and the rollup plugin is still releasing updated versions, i.e. v6.1.1 was released May 18, 2024.

On the other hand, tsup seems actively maintained but has many issues and looks a bit fragile.

We need to consider a little more to use tsup or not?

Thanks for your info.

@crutchcorn
Copy link
Contributor

tsup itself seems actively maintained but has many issues and looks a bit fragile.

This was our experience as well 100%.

FWIW, I know you were mentioning not wanting to migrate towards TypeScript, but it does enable easier generation of these kinds of CJS style typings because you can leverage .d.ts generation rather than trying to transform existing .d.ts files into another kind of .d.cts file.

Moreover, I had taken a stab at doing some of the migration just to get a sense of the effort (strict mode et al) and got about halfway through. While there's some code that'd be pretty gnarly in TypeScript in cssColor (especially around string vars being reused as numbers later on), it's worth mentioning that during the migration I already found:

  • One small bug
    • Missing an if (cacheKey) check in colorToHwb
  • Some broken tests
    • All of which are just minor typos in the assert clause
  • Some typing inaccuracies
    • parseRgb being typed as Array<string | number> | string instead of Array<string | number> | (string | null) like the other parse functions

Which I believe the migration suggestion in #38 (Config, Vitest, TypeScript source files, et al) would help mitigate in the long run while making the code and packaging more manageable.

I'd be happy to finish the migration if you'd like, but also understand if you'd rather not. Let me know :)

PS, on a personal note; I apologize if I came in hot. Never my intention to ruffle feathers but just wanted to jump in and help as I saw most impactful in the long-haul

@asamuzaK
Copy link
Owner Author

I'd be happy to finish the migration if you'd like

Okay, let's move on.
Thanks for your proposal.
I'll wait for your PR.

I apologize if I came in hot

No worries at all.
I just wanted reasonable and detailed facts to make the decision.

@asamuzaK
Copy link
Owner Author

asamuzaK commented Jan 12, 2025

One note:
Please provide an esm (minified) version with all dependencies bundled in a single file for browsers, i.e. dist/esm/css-color.min.js. (output dir and filename can be changed)
This project was a part of WebExtensions which I create, sidebarTabs, and I need that bundled file.

asamuzaK pushed a commit that referenced this issue Jan 13, 2025
Fixes #39, fixes #38, fixes #36, closes #40, closes #37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants