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

remove change in type of original Options from remark-rehype #6211

Closed

Conversation

pedro199288
Copy link

Changes

  • What does this change?
    This PR removes a change that was introduced in the type of original Options from remark-rehype package. This was done in this PR make Remark rehype options available in astro config #4138 in packages/markdown/remark/src/types.ts.
    The reason for this PR is that when passing a valid configuration for remark-rehype (that actually works correctly), typescript is returning an error.

Before the PR change:
image

The above error is:

Type '{ heading: (state: Parameters<Handler>[0], node: Parameters<Handler>[1], parent: Parameters<Handler>[2]) => { type: string; tagName: string; properties: { someProp: string; }; children: never[]; }; }' is not assignable to type '(state: State, parent: MdastNodes) => ElementContent[]'.
  Object literal may only specify known properties, and 'heading' does not exist in type '(state: State, parent: MdastNodes) => ElementContent[]'.

After the PR change:
image

No error appears

Maybe I missed something from the original PR but as it is is giving a typescript error with a config that actually works.

Testing

No test was updated because this is just related to typescript

Docs

/cc @withastro/maintainers-docs for feedback!

Haven't add docs but I can if necessary

@changeset-bot
Copy link

changeset-bot bot commented Feb 10, 2023

🦋 Changeset detected

Latest commit: 3e67032

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

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

@github-actions github-actions bot added the feat: markdown Related to Markdown (scope) label Feb 10, 2023
@pedro199288
Copy link
Author

pedro199288 commented Feb 10, 2023

I don't know why that error in github checks appears but it continues appearing even if I left the code as it was before 😕 (at least in my IDE)

@bluwy
Copy link
Member

bluwy commented Mar 6, 2023

The change makes sense to me, but I'm stumped with the TypeScript error too. Looks like it's a bug there: microsoft/TypeScript#42873

If I do:

export type RemarkRehype = Omit<RemarkRehypeOptions, ''>

instead, which is kinda silly, it works. Maybe you can try this and merge up main? I can't seem to merge main locally, there's some sort of history mismatch.

@matthewp
Copy link
Contributor

matthewp commented Mar 7, 2023

@pedro199288 do you plan to still work on this?

@pedro199288
Copy link
Author

@bluwy thanks for the support. What you suggested looks silly, but the fact that it doesn't work without that, also looks silly to me. So I will give a try, thanks! :)

@pedro199288
Copy link
Author

@pedro199288 do you plan to still work on this?

Yeah I will give it a third and last try this week or the next one. If don't manage to fix it I'll let you know

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Mar 8, 2023
@pedro199288
Copy link
Author

I've tried what @bluwy suggested but it didn't work just with that. I've changed two files in my last commit. Curiously, only doing one of these changes didn't fix the issue with the build step but adding the two changes did so (I don't know why).

You can see that I set preserveSymlinks: true but probably there is a reason why this was with its default to false? That "solution" was one of the suggestions in the typescript bug previously mentioned microsoft/TypeScript#42873 (comment) but I'm not sure if it is a valid solution for this project

@matthewp
Copy link
Contributor

@pedro199288 all tests are passing now, are you saying this isn't working? Or just that you're unsure about the option you added?

@pedro199288
Copy link
Author

@matthewp yeah it works fine for me also. It's just that I'm not sure if the change in tsconfig might be a problem for whatever reason that I'm not aware of. Other than that it looks good to me :)

@natemoo-re natemoo-re force-pushed the fix-remark-rehype-typescript branch from d128462 to 62078c4 Compare June 8, 2023 21:36
@natemoo-re
Copy link
Member

Hey @pedro199288, sorry for the delay on this! This change seems good to me, but I agree that the "preserveSymlinks": true and Omit changes are a bit concerning.

Happy to try to resolve these if you're still interesting in getting this change in.

@pedro199288
Copy link
Author

Hey @natemoo-re ! sorry for the late response. But yeah sure, it'd be nice if you can help with this

@ematipico
Copy link
Member

@pedro199288 I don't think the team has the bandwidth to help you with this PR. It's OK if you aren't interested anymore to this PR

@bluwy bluwy mentioned this pull request Nov 21, 2023
@natemoo-re
Copy link
Member

We're planning to fix this in #9147 and release in Astro 4.0.

@bluwy
Copy link
Member

bluwy commented Nov 22, 2023

Closing as #9147 is merged

@bluwy bluwy closed this Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants