Skip to content

Usage without gatsby #157

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

Merged
merged 5 commits into from
Jul 6, 2021

Conversation

codepunkt
Copy link
Contributor

Fixes #99

Copy link
Owner

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

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

Thanks so much for taking this on! This looks pretty uncontroversial to me. Code sharing between this and the gatsby equivalent could definitely be better—are there opportunities where it makes sense to extract some stuff, or are there subtle difference interleaved throughout the plugin body?

I think longer term it would make sense if this were in a separate package that gatsby-remark-vscode depended on (it does have “gatsby” in its name after all), but I’m totally happy to try this out here for now, especially given that I haven’t had time to spend maintaining this myself lately. Just happy for someone else to keep it alive.

CI is only failing due to prettier. npm run format should do the trick.

FYI I’m going to be on a road trip off and on for the next week, so I might be slow to respond, but will try to check in and get this thing merged. Do you want to add something to the README as well?

@codepunkt
Copy link
Contributor Author

codepunkt commented Jun 30, 2021

Thanks so much for taking this on! This looks pretty uncontroversial to me. Code sharing between this and the gatsby equivalent could definitely be better—are there opportunities where it makes sense to extract some stuff, or are there subtle difference interleaved throughout the plugin body?

There are a few subtle differences interleaved and I also made one or two minor adjustments to be working in an ESM package (require.main.filename not being defined). I agree that code sharing between this and the gatsby equivalent could be way better, but I don't really know what the best approach would be. Maybe, if you'll have the time in the future, we can pair on this.

I think longer term it would make sense if this were in a separate package that gatsby-remark-vscode depended on (it does have “gatsby” in its name after all), but I’m totally happy to try this out here for now, especially given that I haven’t had time to spend maintaining this myself lately. Just happy for someone else to keep it alive.

I would like that aswell! For the time being, I'm happy that I managed to get this working at all, though 😁

CI is only failing due to prettier. npm run format should do the trick.

Will do.

FYI I’m going to be on a road trip off and on for the next week, so I might be slow to respond, but will try to check in and get this thing merged. Do you want to add something to the README as well?

I'll add a tiny bit to the README and would love to get this merged/released. Enjoy your road trip!

@codepunkt codepunkt requested a review from andrewbranch June 30, 2021 15:09
@andrewbranch
Copy link
Owner

Is @wooorm’s feedback in #99 actionable here, @codepunkt?

@codepunkt
Copy link
Contributor Author

Probably, but that sounds like a general improvement to the library that’s not specific to this PR.

@codepunkt
Copy link
Contributor Author

@andrewbranch Not sure whether or not you're still on your roadtrip - I hope everything's fine!

I added documenteation to the README, formatted stuff via linter and extracted the remarkPlugin to its own file.

@andrewbranch
Copy link
Owner

andrewbranch commented Jul 5, 2021

Probably, but that sounds like a general improvement to the library that’s not specific to this PR.

That might be true, but it would be news to me. The impression I got when writing this originally is that if I wanted to inject HTML into a Markdown file, I just had to stringify that HTML and put it in an HTML node—in other words, that I couldn't embed an HTML AST inside the Markdown AST. If that's wrong, I would have built this super differently 😅

Agreed that probably shouldn't be part of this PR, but I wanted to get a better understanding of the problem and the suggestion before committing to these two plugins living in the same place. If the Remark plugin is going to be permanently held back due to the gatsby-remark plugin needing stringified HTML, this project seems a little less worthwhile to me.

@wooorm
Copy link

wooorm commented Jul 6, 2021

due to the gatsby-remark plugin needing stringified HTML

I believe Gatsby has also always supported the embedded AST, so I don’t think there is any holding back!

@andrewbranch andrewbranch merged commit c48dd09 into andrewbranch:master Jul 6, 2021
@codepunkt codepunkt deleted the usage-without-gatsby branch July 6, 2021 19:58
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.

Usage without gatsby
3 participants