[gatsby-transformer-javascript-frontmatter] Frontmatter export breaks HMR

mosesoak commented 2 years ago

Preliminary Checks

[X] This issue is not a duplicate. Before opening a new issue, please search existing issues: https://github.com/gatsbyjs/gatsby/issues
[X] This issue is not a question, feature request, RFC, or anything other than a bug report directly related to Gatsby. Please post those things in GitHub Discussions: https://github.com/gatsbyjs/gatsby/discussions

Description

Hot module reload (HMR) doesn't work, using gatsby-transformer-javascript-frontmatter plugin when javascript or typescript files export frontmatter.

This issue has been around since at least Gatsby 3, and continues in Gatsby 4.

Reproduction Link

https://github.com/yallllc/repro-js-frontmatter-hmr

Steps to Reproduce

Install gatsby-transformer-javascript-frontmatter plugin
Export frontmatter from a page
Make a visual change -- style or a jsx -- and see if the page updates on save

Expected Result

Page updates

Actual Result

Page doesn't update.

If the frontmatter export is commented out, HMR will start working again.

Environment

System:
    OS: Linux 5.4 Ubuntu 22.04 LTS 22.04 (Jammy Jellyfish)
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 16.14.2 - ~/.nvm/versions/node/v16.14.2/bin/node
    Yarn: 1.22.18 - ~/.nvm/versions/node/v16.14.2/bin/yarn
    npm: 8.5.0 - ~/.nvm/versions/node/v16.14.2/bin/npm
  npmPackages:
    gatsby: ^4.15.0 => 4.15.1 
    gatsby-transformer-javascript-frontmatter: ^4.15.0 => 4.15.0

Config Flags

No response

LekoArts commented 2 years ago

Hi!

This is "working" as expected as Fast Refresh only allows one export per file to work correctly. See https://www.gatsbyjs.com/docs/reference/local-development/fast-refresh/

The documentation at https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-javascript-frontmatter/README.md needs an update to point this limitation out and link to the Fast Refresh documentation.

mosesoak commented 2 years ago

@LekoArts Thanks for the reply. Just to clarify, are you saying that because the JS Frontmatter plugin requires you to export frontmatter, that it will always break Fast Refresh?

Or is there a syntax that can be used so the plugin doesn't break core Gatsby functionality? Thanks

LekoArts commented 2 years ago

JS Frontmatter plugin requires you to export frontmatter, that it will always break Fast Refresh?

Yes, that's correct. Fast Refresh only allows one export per file.

Or is there a syntax that can be used so the plugin doesn't break core Gatsby functionality? Thanks

If you look at https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-transformer-javascript-frontmatter/src/gatsby-node.js you see that it uses babel to look for these exports. So you can only use another syntax when the plugin is updated which would also be a breaking change.

As the plugin only has around 800 downloads a week we won't spend any time on it in the foreseeable future fixing this, however if you or anyone else wants to put up a PR fixing this limitation we're happy to review it. If not, that's also okay -- then for now the README should be updated.

The export syntax would need to be changed to just a definition with a pretty clear name so that you can easily find it via Babel.

mosesoak commented 2 years ago

I see. It probably has low downloads because it breaks Gatsby fast refresh. 🤷 In general it's a super useful plugin that our team would love to make use of. Not sure we'll have the bandwidth to contribute but thanks for your replies!

mosesoak commented 2 years ago

@LekoArts I got it working! Took your suggestion and modified the plugin so it works without export.

https://github.com/yallllc/repro-js-frontmatter-hmr/blob/custom-js-plugin/plugins/gatsby-transformer-javascript-frontmatter/gatsby-node.js#L88-L99

One caveat is that unless the frontmatter object is used in the component directly, you need to disable eslint for the line. (I couldn't think of any other standard js way to declare it without an export, since React FC's can't use statics.) https://github.com/yallllc/repro-js-frontmatter-hmr/blob/custom-js-plugin/src/pages/index.js#L4-L18

After porting this to our current project, fast refresh and frontmatter queries are working together now!

(I could potentially PR this to Gatsby if you think it's a decent approach that might be valuable for others. Thanks again for your efforts with the community!)

mosesoak commented 2 years ago

@LekoArts Side question: can you give a little more background on why adding a named export to a page node breaks Fast Refresh -- is that a feature or a limitation? Feel free to point me to a discussion or code reference as I'd love to help your team look into it if it's something to be solved. Thanks!

LekoArts commented 2 years ago

It's a limitation of Fast Refresh, you can also learn more here: https://github.com/facebook/react/issues/16604#issuecomment-528663101

I'm out this week, I can look at it more next week.

toxsick commented 2 years ago

@LekoArts I got it working! Took your suggestion and modified the plugin so it works without export.

https://github.com/yallllc/repro-js-frontmatter-hmr/blob/custom-js-plugin/plugins/gatsby-transformer-javascript-frontmatter/gatsby-node.js#L88-L99

One caveat is that unless the frontmatter object is used in the component directly, you need to disable eslint for the line. (I couldn't think of any other standard js way to declare it without an export, since React FC's can't use statics.) https://github.com/yallllc/repro-js-frontmatter-hmr/blob/custom-js-plugin/src/pages/index.js#L4-L18

After porting this to our current project, fast refresh and frontmatter queries are working together now!

(I could potentially PR this to Gatsby if you think it's a decent approach that might be valuable for others. Thanks again for your efforts with the community!)

@mosesoak It would be awesome if you could put this into a PR, facing the same problem.

regards

mosesoak commented 2 years ago

@toxsick I will once I get some time, but for now feel free to grab the modified extension from my repo branch above! All you have to do is copy the plugins folder to your project, remove the official one from your package json (e.g. yarn remove to be sure the node module gets deleted), and leave it set up in the gatsby-config as normal.

mosesoak commented 2 years ago

It's a limitation of Fast Refresh

@LekoArts So what's going is that React came up with this implementation, which was designed to create a boundary where only react files would be hot-reloaded, and asked the various packager projects to use that function. Webpack implemented it; Gatsby uses Webpack's implementation. Is that right?

(Interestingly someone wrote an early response to their RFC saying this would happen if the proposed strategy were adopted, and it has, so it doesn't seem like the React team thought it through very well before turning it over to the many packager vendors. It also looked like maybe Vite has already started working around it.)

Given all this, changing the Gatsby plugin to accept a non-exported frontmatter (which is working for me locally now, with my changes) seems like a good way to deal with this. I'll consider doing a PR on it when I have time.

LekoArts commented 2 years ago

Yeah, feel free to PR your changes to the main plugin and then we can release a new major (as I'd like to remove the support for export const frontmatter as it doesn't work).

github-actions[bot] commented 2 years ago

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

mosesoak commented 2 years ago

Sorry I haven't had time to address this issue yet but I do plan to contribute soon -- please keep open! Thanks

github-actions[bot] commented 2 years ago

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

mosesoak commented 2 years ago

Again this is still on my radar sorry for the delays

github-actions[bot] commented 2 years ago

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

mosesoak commented 2 years ago

keepalive. still want to do this soon.

github-actions[bot] commented 2 years ago

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 days of inactivity. It’s been at least 20 days since the last update here. If we missed this issue or if you want to keep it open, please reply here. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

github-actions[bot] commented 1 year ago

Hey again!

It’s been 60 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it. Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to comment on this issue or create a new one if you need anything else. As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

theresasogunle commented 1 year ago

Is anyone working on this yet? I’d like to pick this up

EyaChebbi commented 1 year ago

Hello is anyone working on this?

mosesoak commented 1 year ago

Hi @EyaChebbi sorry for the long delay. I actually will have some time I can put into this at the start of April.

arkzuse commented 1 year ago

Hello @mosesoak if you are not free, then I would like to work on this issue

mosesoak commented 10 months ago

Please doOn Sep 8, 2023, at 10:24 PM, arkzuse @.***> wrote: Hello @mosesoak if you are not free, then I would like to work on this issue

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

gatsbyjs / gatsby