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

Update tsconfig.json, use node:test #21

Merged
merged 8 commits into from
Feb 6, 2023

Conversation

ChristianMurphy
Copy link
Member

@ChristianMurphy ChristianMurphy commented Jan 17, 2023

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Upgrades hast-util-raw.
The latest version of hast-util-raw includes types fixes which would help react-markdown which currently needs skipLibCheck to ignore the typing issue.

Upgrades typescript to use node16 resolution for better ESM support.
Replaces tape with node:test and node:assert

@ChristianMurphy ChristianMurphy added the 🧑 semver/major This is a change label Jan 17, 2023
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Jan 17, 2023
@wooorm
Copy link
Member

wooorm commented Jan 17, 2023

fermium probably?

@ChristianMurphy
Copy link
Member Author

Good idea! Updated CI to fermium

test.js Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (038f8be) compared to base (8e413e4).
Patch has no changes to coverable lines.

❗ Current head 038f8be differs from pull request most recent head fc02c20. Consider uploading reports for the commit fc02c20 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #21   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           21        21           
=========================================
  Hits            21        21           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@wooorm wooorm left a comment

Choose a reason for hiding this comment

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

I’m not sure I want to do majors of rehype plugins etc now.

I’ve gotten all the hast utilities in shape, but I’m still expecting a month or two of work to get all the micromark/mdast stuff in order, so that we can do one big major run through the ecosystem.

Looking at react-markdown and skipLibCheck, there seem to be tons of other errors (also in esbuild), so if dropping skipLibCheck is the goal, I don’t think just this PR will get it there.

PR otherwise looks great!

package.json Show resolved Hide resolved
@ChristianMurphy
Copy link
Member Author

I’ve gotten all the hast utilities in shape, but I’m still expecting a month or two of work to get all the micromark/mdast stuff in order, so that we can do one big major run through the ecosystem.

Sounds good! 👍

Looking at react-markdown and skipLibCheck, there seem to be tons of other errors (also in esbuild), so if dropping skipLibCheck is the goal, I don’t think just this PR will get it there.

Yep, it is a broader goal/ideal.
Which may not be completely reached (some errors are coming from outside the ecosystem).
This alone won't fix all the issues, but it will check off one of the errors.

@wooorm
Copy link
Member

wooorm commented Jan 18, 2023

Could you drop the hast-util-raw upgrade for now? I think it’s a cleaner PR if these changes are separated from that, and then I can already do a release without a major!

@ChristianMurphy ChristianMurphy removed the 🧑 semver/major This is a change label Jan 19, 2023
@ChristianMurphy
Copy link
Member Author

Could you drop the hast-util-raw upgrade for now? I think it’s a cleaner PR if these changes are separated from that, and then I can already do a release without a major!

Sure, walked back in 038f8be

@ChristianMurphy ChristianMurphy changed the title upgrade hast util raw to version 8, use node:test and node:assert TypeScript node16 mode, use node:test and node:assert Jan 19, 2023
test.js Outdated
@@ -35,9 +34,9 @@ test('rehypeRaw', (t) => {
'</div>'
].join('\n'),
(error, file) => {
Copy link
Member

Choose a reason for hiding this comment

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

This is the execution order now:

  1. the test runs
  2. The unified processor is started
  3. The test succeeds
  4. The unified processor calls a callback
  5. Assertions are made

Previously tape could use the t context to determine how many assertions were expected, and wait for those. This is no longer the case.

IMO the cleanest solution is to either:

  • use the unified promise API, and async / await syntax for this test.
  • use the unified sync API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it has been reordered in that way.
I tried adding assert.fail() inside the callback to see if it would wait long enough for it to fail.
It does fail.

I'm open to making it async if that would be more clear for folks.
But I don't think the change is required for the tests to function.

Copy link
Member

Choose a reason for hiding this comment

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

I think Remco here is correct, due to plan. That makes tape run async and wait until there are two assertions.

I think what’s happening now is just uncaught exceptions (because assert throws errors, tape doesn’t). While it “works” (because uncaught exceptions still fail the process), I don’t think that’s a good idea?

So I believe your assert.fail causes an uncaught exception?

I like using async/await now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in fc02c20

@wooorm
Copy link
Member

wooorm commented Feb 6, 2023

missing eol somewhere?

@wooorm wooorm changed the title TypeScript node16 mode, use node:test and node:assert Update tsconfig.json, use node:test Feb 6, 2023
@wooorm wooorm merged commit 7bfa975 into rehypejs:main Feb 6, 2023
@wooorm wooorm added the 💪 phase/solved Post is done label Feb 6, 2023
@wooorm
Copy link
Member

wooorm commented Feb 6, 2023

Thanks!

@github-actions

This comment has been minimized.

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 6, 2023
@ChristianMurphy ChristianMurphy deleted the hast-util-raw-8 branch February 6, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

4 participants