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

Default theme shouldn't fetch icons from CDN #7809

Closed
ojab opened this Issue Oct 4, 2017 · 18 comments

Comments

Projects
None yet
8 participants
@ojab
Copy link

ojab commented Oct 4, 2017

Version

2.13.4

Environment

--

Reproduction link

@icon-url : "https://at.alicdn.com/t/font_zck90zmlh7hf47vi";

Steps to reproduce

--

What is expected?

antd shouldn't fetch anything from network

What is actually happening?

Right now icons are fetched from CDN


  1. CDN (and external network in general) can be unavailable due to various reasons
  2. Information leakage (IPs/Referer/etc) is a security issue and it definitely shouldn't happen in default configuration

I assume that default iconset should be bundled into antd or provided via separate package/dependency.

@nikogu

This comment has been minimized.

Copy link
Contributor

nikogu commented Oct 5, 2017

Using the cdn can make your project to small. and the cdn address will not be broken. It's very reliablly.

The cdn services service provider is alibaba.

@afc163

This comment has been minimized.

@afc163 afc163 closed this Oct 6, 2017

@ojab

This comment has been minimized.

Copy link
Author

ojab commented Oct 6, 2017

Well, project size is mostly irrelevant:

  • icons size is negligible compared to the sum of other npm packages in typical project
  • client should download them anyway, no matter where they are located, so project size as seen by the client is the same

CDN is not so reliable, as can be seen in #1070

Anyway, security concern still stands.

I know that antd can be configured to serve locally hosted iconfont, but this issue is about default settings and I think that it should be reopened/fixed.

@martijntraa

This comment has been minimized.

Copy link

martijntraa commented Nov 27, 2017

I agree with @ojab that:

  • this is a security concern (it is for the projects/clients I work for);
  • that the default should be not to use a CDN, with a seperate example to explain how to use antd with a CDN;
  • that project size doesn't seem to be a relevant issue.
@davschne

This comment has been minimized.

Copy link
Contributor

davschne commented Jan 4, 2018

My organization is using antd in one of our applications, and we recently discovered an intermittent (and thus hard to diagnose) bug where an icon had disappeared. We discovered that the request for the anticon font on Alibaba was timing out, so it seems it's not quite as reliable as you may believe. We ended up hosting the font files as static resources locally, but doing so was not trivial. Please, please, please don't force users of this wonderful library to rely on an unreliable CDN (or else have to resort to advanced configuration) for something as small and simple as a font file.

@tibdex

This comment has been minimized.

Copy link
Contributor

tibdex commented Jan 20, 2018

Hi Ant Design team and thank you for this beautiful library. I agree with all the previous concerns and would like antd to work offline too.

I'm trying to make a PR to use a local icon font but it's not very easy to understand how your build work with your webpack configuration spread across webpack.config.js and getWebpackConfig.js in https://github.com/ant-design/antd-tools. I'm currently getting nasty webpack loader errors when trying to have the font files end up in dist.

Before I invest more time in this, can you confirm that you would be willing to change the default theme to stop using the Alibaba CDN and ship the icon font files with the library instead?

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented Jan 22, 2018

I think the initial purpose of using CDN fonts is designed to simplify the process of using antd.

And change this behavior will introduce a breaking change.

@ojab

This comment has been minimized.

Copy link
Author

ojab commented Jan 22, 2018

Another package w/ fonts in dependencies and @icon-url from that package (or even packaging fonts into antd itself) is no more difficult than using font from CDN.

This change will be breaking for cdnjs/unpkg/etc users if fonts will be packaged separately, but it probably can be solved by some kind of fallback (to CDN) for the current major version.

Anyway, technical moments are unrelated unless there is an agreement that CDN usage out of the box is indeed an issue.

@tibdex

This comment has been minimized.

Copy link
Contributor

tibdex commented Jan 29, 2018

The initial purpose of using CDN fonts is designed to simplify the process of using antd.

It might make it simpler for some users but it also prevents a lot of projects required to work off-line or without making requests to chinese servers to use Ant Design without hacks.

I tried to mitigate the problem by not using the <Icon /> component at all in my project but the fonts are eagerly loaded from the CDN so I cannot use any Ant Design component without this undesirable network request being made.

I can see two potential improvements:

  1. Use local icon font assets.
  2. Do the right thing and use SVG instead of an icon font. GitHub has a convincing blog post on why this is a superior approach. This solution solves the CDN problem without making Ant Design harder to use since SVG is supported natively by React and the icon font is already abstracted behind the <Icon /> React component.

What do you think of the last technique?

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented Jan 30, 2018

Proposal 2 looks like a viable option, but it's still a breaking change, we may revisit this issue when when we plan on 4.0

@tibdex

This comment has been minimized.

Copy link
Contributor

tibdex commented Apr 30, 2018

@yesmeck, I don't see how moving the icons from a font to embedded SVG would be a breaking change. The only visible change would be the absence of external HTTP requests which is a good thing.

The way I see it, it would make things easier for you as you wouldn't have to maintain the local-iconfont package anymore.

Besides, you already have the icons in SVG format. Indeed, it's the format used when you search for Ant Design icons on http://iconfont.cn/ and also the one imported here:

url('@{icon-url}.svg#iconfont') format('svg');

Thus, the only thing to do to implement my "proposal 2" would be to split this SVG file into multiple React components (which is doable since React has full support for SVG) and use the existing <Icon /> component to render the correct one.

If I were to submit a PR with these changes, would you consider approving it and merging it as a minor or patch release (without waiting for 4.0)?

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented May 1, 2018

We can't style SVG with font-size, so I think it's a breaking change.

@tibdex

This comment has been minimized.

Copy link
Contributor

tibdex commented May 1, 2018

The "Align SVG Icons to Text and Say Goodbye to Font Icons" article explains how to scale SVGs like an icon font with the "1em technique".

I think we could use this, what's your opinion Wei?

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented May 2, 2018

@tibdex Thanks for the link, I have some more concerns:

  1. How can we support line-height which people may already used for font icon?
  2. If we don't change the API of Icon component, do we need to bundle all SVG icons to production build?
@tibdex

This comment has been minimized.

Copy link
Contributor

tibdex commented May 2, 2018

  1. The article I linked to in my last comment also talks about line-height. Its Codepen is using it too. Is this what you're thinking about?
  2. Yes <Icon /> would import all the SVG icon components and render the correct one with a switch based on the type prop. I know it's not ideal because it will increase the library size but it's the only way to do it without introducing breaking change or without relying on dynamic import().
    In the long term, we should probably do it the way Material UI does it by using one React component per icon. Then we could leverage the babel-plugin-import technique to export all the icon components from a centralized icons/index.js module without impacting the bundle size.

I don't know if you've read the links listed at the end of the "Align SVG Icons to Text and Say Goodbye to Font Icons" article but they are really convincing. Especially these ones:

I believe it's worth switching to SVG icons in one of the next minor releases as it will make the icons sharper and the library significantly easier to use off-line. Shall we try?

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented May 3, 2018

@tibdex Yes, I've read those links, SVG does look good for icons, I just want to make sure it's smoothly and don't break things if we switch to SVG.

@yesmeck

This comment has been minimized.

Copy link
Member

yesmeck commented May 3, 2018

@tibdex Could you create a separator issue?

@felops

This comment has been minimized.

Copy link

felops commented Jun 20, 2018

Could anyone change the "@icon-url" following the documentation? I am following the step-by-step guide for create-react-app (https://ant.design/docs/react/use-with-create-react-app#Customize-Theme) and all my less modifications was working well, but I am not able to set "@icon-url". Is that an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.