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

refactor: replace RuleType enum with a const (#539) #594

Merged
merged 3 commits into from
Aug 18, 2024

Conversation

roger-schaer
Copy link
Contributor

@roger-schaer roger-schaer commented Jul 4, 2024

Summary

This PR resolves #539 by replacing the RuleType enum with a const object.

Description

As mentioned & discussed in #539, the current way of declaring the RuleType as an enum causes problems when using isolatedModules in tsconfig.json.

The solution, as described on the TypeScript website, is to simply replace the enum with a simple object const, and declare a type based on its keys & values. Additional updates were needed to correctly use types (and not values) in the various interfaces defined for node types.

This worked correctly, but there were still issues when running tests using Jest with the modifications made in index.tsx. Therefore, an additional change was made in index.cjs.tsx in order to export the RuleType explicitly (as is done with the compiler function).

Testing

Since the updated code is functionally equivalent to the existing version & all existing unit tests pass correctly, it was not clear what type of test could be added.

Copy link

changeset-bot bot commented Jul 4, 2024

🦋 Changeset detected

Latest commit: ca37142

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
markdown-to-jsx Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@roger-schaer
Copy link
Contributor Author

Hi @quantizor, just wanted to check if you would have time in the coming weeks to check out this PR, it would unblock some things on my end if this could be merged. Thanks in advance and have a nice summer!

@quantizor
Copy link
Owner

Hi @quantizor, just wanted to check if you would have time in the coming weeks to check out this PR, it would unblock some things on my end if this could be merged. Thanks in advance and have a nice summer!

Hey Roger! I will try to make time, thanks for the reminder. Lots going on personally at the moment.

@quantizor quantizor self-assigned this Jul 16, 2024
@roger-schaer
Copy link
Contributor Author

Hi @quantizor, just wanted to check if you would have time in the coming weeks to check out this PR, it would unblock some things on my end if this could be merged. Thanks in advance and have a nice summer!

Hey Roger! I will try to make time, thanks for the reminder. Lots going on personally at the moment.

No worries, thank you for your answer and your help!

@zivkovicn
Copy link

@quantizor hello, any plans in merging this? thanks

Copy link
Owner

@quantizor quantizor left a comment

Choose a reason for hiding this comment

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

Thank you for doing this and apologies it took me some time to circle back to it.

@quantizor quantizor merged commit 553a175 into quantizor:main Aug 18, 2024
5 checks passed
@quantizor quantizor mentioned this pull request Aug 18, 2024
6 tasks
@roger-schaer
Copy link
Contributor Author

Thanks a lot @quantizor, I tested it now and it works perfectly with the new version!

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.

Cannot access ambient const enums when 'isolatedModules' is enabled.
3 participants