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.

Sign up for GitHub

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

Jump to bottom

fix(gatsby-plugin-mdx): support import shorthand syntax #26198

Merged
merged 1 commit into from Aug 3, 2020
Merged

fix(gatsby-plugin-mdx): support import shorthand syntax #26198

merged 1 commit into from Aug 3, 2020

Conversation

pvdz
Copy link
Contributor

@pvdz pvdz commented Aug 3, 2020

The "shorthand" syntax is an import that only imports a path, without declaring any symbols:

import "./foo.css"

This happened in the real world so we have to support it.

This fixes #25734

@pvdz pvdz requested a review from a team as a code owner August 3, 2020 10:32
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Aug 3, 2020
@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 3, 2020 •
edited

Gatsby Cloud Build Report

client-only-paths

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 🔶 85
Best Practices 💚 100
SEO 🔶 70

🔗 View full report

@pvdz
fix(gatsby-plugin-mdx): support import shorthand syntax
10da200 
The "shorthand" syntax is an import that only imports a path, without declaring any symbols:

```js
import "./foo.css"
```

This happened in the real world so we have to support it.

This fixes #25734
@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 3, 2020 •
edited

Gatsby Cloud Build Report

using-styled-components

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 💚 90
Best Practices 💚 100
SEO 💚 90

🔗 View full report

@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 3, 2020 •
edited

Gatsby Cloud Build Report

using-reach-skip-nav

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 100
Accessibility 💚 100
Best Practices 💚 100
SEO 🔶 82

🔗 View full report

@gatsby-cloud-staging
Copy link

gatsby-cloud-staging bot commented Aug 3, 2020 •
edited

Gatsby Cloud Build Report

gatsby-master

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 1m

Performance

Lighthouse report

Metric Score
Performance 💚 97
Accessibility 🔶 87
Best Practices 💚 93
SEO 🔶 73

🔗 View full report

@gatsby-cloud
Copy link

gatsby-cloud bot commented Aug 3, 2020

Gatsby Cloud Build Report

gatsby

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 24m

laurieontech
laurieontech reviewed Aug 3, 2020
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

This looks fine and glad we got this edge case. But I’m wondering if we should look at the parsing for imports in mdx directly? It’d be great if they were the same. I can look at the 2.0 stuff this morning to see if it’s worth lifting.

@pvdz
Copy link
Contributor Author

pvdz commented Aug 3, 2020

@laurieontech mdx itself should/could definitely handle this better. For the time being, our plugin does it. Before it would off-load that to Babel but since we don't do that anymore, we need to be specific ourselves. I missed (or well, didn't expect) this particular case of the import. Hopefully that's the last of them.

If this approach can be brought into @mdx-js that'd definitely be my preference too. Another parsing step we can eliminate, maybe :)

laurieontech
laurieontech approved these changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Let's get this in. Can make changes later if necessary but want to fix.

@pvdz pvdz added bot: merge on green Gatsbot will merge these PRs automatically when all tests passes topic: MDX and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Aug 3, 2020
@gatsbybot gatsbybot merged commit 0df7977 into master Aug 3, 2020
@pvdz pvdz added the type: bug An issue or pull request relating to a bug in Gatsby label Aug 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the mdx-import-shorthand branch August 3, 2020 13:00
@pvdz
Copy link
Contributor Author

pvdz commented Aug 3, 2020

Published in gatsby-plugin-mdx@1.2.32

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurieontech laurieontech laurieontech approved these changes

Assignees
No one assigned
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

error "gatsby-plugin-mdx" threw an error while running the onCreateNode lifecycle:
3 participants
@pvdz @laurieontech @gatsbybot