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

Migration to Tanstack Config #41

Merged
merged 40 commits into from
Jan 13, 2025
Merged

Conversation

crutchcorn
Copy link
Contributor

@crutchcorn crutchcorn commented Jan 12, 2025

This PR:

Future TODOs

This code was not particularly type-safe from the start, so there's a myriad of type-casting and ignoring TS that's not ideal.

At some point we should:

  • Ban ! cast usage
  • Ban as type casting
  • Fix export types to be more strict
  • Remove object and any usage
  • Move Array types to tuple types
  • Refactor code that overrides existing variable types to dedicated single-use types
  • Review tests to use assert instead of expect

src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/convert.ts Outdated Show resolved Hide resolved
src/js/convert.ts Outdated Show resolved Hide resolved
src/js/resolve.ts Show resolved Hide resolved
Copy link
Owner

@asamuzaK asamuzaK left a comment

Choose a reason for hiding this comment

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

I want to keep tests with assert.

tests/color.test.ts Outdated Show resolved Hide resolved
vite.browser.config.ts Outdated Show resolved Hide resolved
@asamuzaK
Copy link
Owner

About tests:

  • Rename *.test.js to *.test.ts
  • In source, fix only imports:
    e.g. color.test.ts
    /* snip */
    import { strict as assert } from 'node:assert';
    import { describe, it } from 'vitest';
    
    /* test */
    import * as color from '../src/js/color'; // remove file extension

Will there be any problems if we fix tests as above?

@crutchcorn
Copy link
Contributor Author

crutchcorn commented Jan 13, 2025

Also, I don't think that Vitest works with external asserts and, transparently, I'm not going to redo all of that work I did on tests.

Okay, please go with it.
Let's think about it later.

The first priority is to fix the issues.
It's OK to postpone things that we can think about later.

@crutchcorn
Copy link
Contributor Author

OK all type casts should now be fixed as well @asamuzaK - this PR should be ready to review again

@crutchcorn
Copy link
Contributor Author

Also, I tested this PR against my currently broken production usage of cssstyle and confirmed that this fixes it:

Before

image

After

image

image

src/js/color.ts Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Outdated Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
src/js/color.ts Show resolved Hide resolved
@crutchcorn
Copy link
Contributor Author

crutchcorn commented Jan 13, 2025

@asamuzaK can you handle the nit comments on your own after merging? It would be great to merge this so we can be unblocked on our projects using jsdom

I think this PR is ready to merge, since it's been tested and it seems like all review comments have been addressed

src/js/convert.ts Outdated Show resolved Hide resolved
src/js/css-calc.ts Show resolved Hide resolved
src/js/css-calc.ts Show resolved Hide resolved
src/js/css-calc.ts Show resolved Hide resolved
src/js/css-calc.ts Show resolved Hide resolved
src/js/css-var.ts Outdated Show resolved Hide resolved
src/js/resolve.ts Show resolved Hide resolved
src/js/resolve.ts Show resolved Hide resolved
@asamuzaK
Copy link
Owner

asamuzaK commented Jan 13, 2025

@asamuzaK can you handle the nit comments on your own after merging? It would be great to merge this so we can be unblocked on our projects using jsdom

Sure.

@crutchcorn
Copy link
Contributor Author

Ready for another round of reviews :)

Copy link
Owner

@asamuzaK asamuzaK left a comment

Choose a reason for hiding this comment

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

I've added reviewing tests in TODO.
Let's merge.
I'll add some fixes after that and release a new version ASAP.

@crutchcorn
Copy link
Contributor Author

Thanks a bunch @asamuzaK! Feel free to tag me when you publish the new version and I'll make a PRs upstream to bump the package version there so JSDom can be unblocked :D

@asamuzaK asamuzaK merged commit e951f00 into asamuzaK:main Jan 13, 2025
@crutchcorn
Copy link
Contributor Author

I see that CI/CD is broken. I can fix that with a follow-up PR real fast

@asamuzaK
Copy link
Owner

@asamuzaK
Copy link
Owner

npm audit report

cross-spawn <6.0.6
Severity: high
Regular Expression Denial of Service (ReDoS) in cross-spawn - GHSA-3xgq-45jj-v275
No fix available
node_modules/execa/node_modules/cross-spawn
execa 0.5.0 - 0.9.0
Depends on vulnerable versions of cross-spawn
node_modules/execa
current-git-branch *
Depends on vulnerable versions of execa
Depends on vulnerable versions of is-git-repository
node_modules/current-git-branch
@tanstack/config *
Depends on vulnerable versions of current-git-branch
node_modules/@tanstack/config
is-git-repository <=1.1.1
Depends on vulnerable versions of execa
node_modules/is-git-repository

I think @tanstack/config need to replace current-git-branch with something.
It's not updated over 6 years.
https://www.npmjs.com/package/current-git-branch?activeTab=code

@crutchcorn
Copy link
Contributor Author

I'll forward to the team, but this shouldn't block merging on this project in particular. current-git-branch is only used in the publish script which is not used here and only on dev mode.

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.

Provide types for cjs
2 participants