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

feat(v2): support for adding relative images and handling broken image links #3069

Merged
merged 5 commits into from
Jul 21, 2020

Conversation

anshulrgoyal
Copy link
Contributor

@anshulrgoyal anshulrgoyal commented Jul 16, 2020

Motivation

Allow the use relative path for images. Closes #3057

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

Added tests

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 16, 2020
@docusaurus-bot
Copy link
Contributor

docusaurus-bot commented Jul 16, 2020

Deploy preview for docusaurus-2 ready!

Built with commit 5725d64

https://deploy-preview-3069--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.

Hey, this looks like a great POC and works fine.

We should at least do the following before merge:

  • add some tests
  • document dogfood this (ie use the feature on the "markdown features" doc page)

Also, one awesome thing is that we can avoid bad image paths at compilation time now.

image

I also wonder, what do you think of supporting both syntaxes?

![](./assets/docusaurus-asset-example-banner.png)
![](assets/docusaurus-asset-example-banner.png)

Also, what about supporting this for assets of the static folder too?

For example we could make this work:

![](/img/docusaurus.png)

And make this fail at compile time:

![](/img/docusaurusTypo.png)

That would be a nice feature and add more safety to the assets system

@slorber slorber linked an issue Jul 20, 2020 that may be closed by this pull request
@anshulrgoyal anshulrgoyal changed the title feat(v2) support for adding relative images feat(v2): support for adding relative images and handling broken image links Jul 20, 2020
@anshulrgoyal
Copy link
Contributor Author

Now it will verify all the ![]() images and check if file exists or not. If the file is missing throw an error and will convert all relative paths to url-loder require statements.

@slorber slorber merged commit 3155dc3 into facebook:master Jul 21, 2020
@slorber
Copy link
Collaborator

slorber commented Jul 21, 2020

looks great, thanks!

@SwaroopH
Copy link

@anshulrgoyal thanks for this. I was using a post build step to manually copy over relative assets. However, I notice that all images are being rendered as base64 urls instead of referring the actual asset. Any clue if this is a feature I need to disable?

@anshulrgoyal
Copy link
Contributor Author

@anshulrgoyal thanks for this. I was using a post build step to manually copy over relative assets. However, I notice that all images are being rendered as base64 urls instead of referring the actual asset. Any clue if this is a feature I need to disable?

Images are mostly loaded using url-loader. If there is a very big image then it will be loaded using file-loader.

@SwaroopH
Copy link

Images are mostly loaded using url-loader. If there is a very big image then it will be loaded using file-loader.

I see, let me figure out a way to disable this for our install. Thanks for the prompt response!

@anshulrgoyal
Copy link
Contributor Author

There is no way to disable it.

@SwaroopH
Copy link

There is no way to disable it.

No, I mean in the core itself. Might make a PR if I can do it in a neat config friendly way.

@anshulrgoyal
Copy link
Contributor Author

But why do u need to refer actual assets?

@SwaroopH
Copy link

But why do u need to refer actual assets?

Better for site performance. Assets get cached by our CDN while html does not (for a good reason).

@anshulrgoyal
Copy link
Contributor Author

ok got it.

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

This is the difference between file-loader vs url-loader. There's a minimum size threshold that we could expose as a configuration option.

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

https://webpack.js.org/loaders/url-loader/

Currently, the limit is 10kb, this is also what Gatby uses and seems a reasonable default value, but we could allow the user to provide what he wants so that assets are not inlined as base64 uris.

If you want to submit a PR, webpack/utils.ts#getFileLoaderUtils

@SwaroopH
Copy link

https://webpack.js.org/loaders/url-loader/

Currently, the limit is 10kb, this is also what Gatby uses and seems a reasonable default value, but we could allow the user to provide what he wants so that assets are not inlined as base64 uris.

If you want to submit a PR, webpack/utils.ts#getFileLoaderUtils

@slorber that's strange, I have a 276kb file here being loaded as base64 URI.

Site: https://new.ethvigil.com/docs/ (running on alpha 59)

Asset: https://github.com/blockvigil/ethvigil-docs/blob/docusaurus-v2/docs/assets/intro/ethvigil_intro.jpg

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

ah yes, I see where this comes from.

image

When transforming the md syntax, we explicitly request url loader to kick in (also permits to avoid conflicts with the ideal image plugin), with no config, so using the md image syntax will always lead to inlined base64 images :/

As a workaround you can use a require() calls for now, as it's documented in the md features page.

Not sure yet what's the best solution to this problem :/ this may require a breaking change in the ideal image plugin so that we can use require calls without forcing a specific loader.

@anshulrgoyal any idea?

@anshulrgoyal
Copy link
Contributor Author

anshulrgoyal commented Jul 27, 2020

@slorber I can add config to specify loader. eg user can provide a function that will transform the argument of require call

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

Not sure to understand what you mean sorry

@anshulrgoyal
Copy link
Contributor Author

node.url
        ? `src={require("${userFunction(node.url)}").default}`
        : ''

@anshulrgoyal
Copy link
Contributor Author

Something like this but its not ideal

@slorber
Copy link
Collaborator

slorber commented Jul 27, 2020

I think we should find and very different solution.

I'd probably prefer having require("./image.png") work out of the box, using the loader we defined in the config, and do something special for the ideal image plugin case, because it's this plugin that forces us to add loader in require calls in the first place.

@davenquinn
Copy link

Is there a way to disable this?

This just caused my CI documentation builds to fail; we aren't managing static assets in git and would prefer to route to them externally via nginx. We have a fairly robust caching setup backed by S3, and would rather continue to handle things that way.

@anshulrgoyal
Copy link
Contributor Author

u can add pathname: to disable file check in front of images. Eg

![image](pathname:///image.png)

@slorber
Copy link
Collaborator

slorber commented Jul 28, 2020

Yes, as I thought this new behavior would annoy some users, I've added the pathname:// escape hatch. Note if you use a protocol like https in the images uris it would also work fine.

@SwaroopH
Copy link

SwaroopH commented Aug 1, 2020

@slorber thanks for fixing it in #3180!

@slorber
Copy link
Collaborator

slorber commented Aug 1, 2020

Np 👍

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: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support collocated images Markdown relative image paths
6 participants