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

fix: allow links to JSON in .md files to be transformed as asset links #4827

Merged
merged 7 commits into from
Jan 23, 2022

Conversation

anthonymccaigue
Copy link
Contributor

@anthonymccaigue anthonymccaigue commented May 21, 2021

closes #3561
It seems to be a common problem that many people are having see:
https://stackoverflow.com/questions/65307533/link-to-static-json-file

Co-authored-by: Anthony McCaigue anthony@nquiringminds.com
Co-authored-by: Alois Klink alois@nquiringminds.com

Motivation

There is a common problem with links to JSON files see:
https://stackoverflow.com/questions/65307533/link-to-static-json-file

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

closes facebook#3561
It seems to be a common problem that many people are having see:
https://stackoverflow.com/questions/65307533/link-to-static-json-file

Co-authored-by: Anthony McCaigue <anthony@nquiringminds.com>
Co-authored-by: Alois Klink <alois@nquiringminds.com>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label May 21, 2021
@netlify
Copy link

netlify bot commented May 21, 2021

✔️ [V2]
Built without sensitive environment variables

🔨 Explore the source changes: 091d97b

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61eccc46c15dcf0007493929

😎 Browse the preview: https://deploy-preview-4827--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented May 21, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 71
🟢 Accessibility 100
🟢 Best practices 93
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-4827--docusaurus-2.netlify.app/

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks for contributing.

I'm not sure to understand what the problem is in the first place, and also why your solution is a good one.

Please give me context so that I understand better.

Also please add an example in one of our dogfooding pages, I suggest to add something in src/pages/markdown-tests.md, so that we are sure it works and that it keeps working over time.

I tried the following and it didn't seem to work

## Link to JSON file

- [manifest md link](@site/static/manifest.json)

- <a href={require("@site/static/manifest.json").default}>manifest jsx link</a>

@@ -122,6 +123,7 @@ export function getBabelOptions({
babelrc: false,
configFile: babelOptions,
caller: {name: isServer ? 'server' : 'client'},
exclude: new RegExp(`"/${STATIC_DIR_NAME}/"`), // exclude static dir
Copy link
Collaborator

@slorber slorber Jun 3, 2021

Choose a reason for hiding this comment

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

Is this problem better solved using babel config? Shouldn't this be solved at the webpack level? (we have a hidden feature to allow using alternate js loaders esbuild, will it work in such case?)

is it normal to have " around the path?

won't this kick in for any /static/ folder (what if users have src/pages/static/xyz for example?)

What about the else case (babelOptions object?)

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Nov 18, 2021

The right fix should be: in Markdown links, JSON extensions should be converted to file-loader calls instead of using Babel loader. This would be done on the mdx-loader side instead.

Edit. it seems JSON links are already converted to file-loader calls...

require('!file-loader?name=assets/files/[name]-[hash].[ext]!../../static/manifest.json').default

I have no idea why it's still attempted to be parsed

@Josh-Cena Josh-Cena added pr: bug fix This PR fixes a bug in a past release. status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. labels Nov 21, 2021
@Josh-Cena
Copy link
Collaborator

I'm taking this over. However this is much harder as it seems, because Webpack 4+ automatically loads JSON as data even with file-loader. (webpack-contrib/file-loader#259, webpack/webpack#6586)

We are migrating to asset modules (#4708) but probably not in a while due to blocking issues about plugin compatibility. I'll see what are the possibilities to solve this. currently build is failing, which is expected.

@Josh-Cena Josh-Cena changed the title fix: fix links to JSON from .md files fix: allow links to JSON in .md files to be transformed as asset links Jan 23, 2022
@Josh-Cena Josh-Cena marked this pull request as draft January 23, 2022 02:42
@Josh-Cena Josh-Cena removed the status: don't merge yet This PR is completed, but we are still waiting for other things to be ready. label Jan 23, 2022
@Josh-Cena Josh-Cena marked this pull request as ready for review January 23, 2022 03:26
@Josh-Cena
Copy link
Collaborator

Fair. I doubt if this is used a lot anyways, and for the rest there's no behavior difference. Will merge it since it's just temporary

@Josh-Cena Josh-Cena merged commit ab1dada into facebook:main Jan 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSON parse fails after updating to 2.0.0-alpha.64
4 participants