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

Theme Color IE issue (improve bundling, etc, for older browsers) #707

Closed
sohrabtaee opened this issue Nov 26, 2018 · 13 comments
Closed

Theme Color IE issue (improve bundling, etc, for older browsers) #707

sohrabtaee opened this issue Nov 26, 2018 · 13 comments

Comments

@sohrabtaee
Copy link

When I add themeColor it breaks in IE11 with this error:

SCRIPT8: The download of the specified resource has failed.
docsify.min.js (1,2752)
@timaschew
Copy link
Member

Please show a code snippet.

@timaschew timaschew added the wait for information something is not clear, waiting for the author of the issue/pr label Feb 26, 2019
@sohrabtaee
Copy link
Author

The problem is that IE11 does not support CSS variables: https://caniuse.com/#feat=css-variables

I added the proper polyfills and it works fine now.

Since Docsify is advertised as "Compatible with IE10+", I guess a polyfill should be added to support CSS variables in IE.

@timaschew timaschew added bug confirmed as a bug and removed wait for information something is not clear, waiting for the author of the issue/pr labels Mar 3, 2019
@anikethsaha
Copy link
Member

shipping polyfills may increase the package size. I think we should update the docs with the guided steps ?

cc @jhildenbiddle @trusktr thoughts?

also, @jhildenbiddle if we need to ship polyfill for css variables from core, thoughts about css-vars-ponyfill ?

@jhildenbiddle
Copy link
Member

Best to have docsify handle loading all necessarily libraries (like poly/ponyfill) as needed via dynamic imports. This keeps the bundle size down, prevents users from having to manage multiple libraries just to get docsify working, and allows the docsify team to easily swap libraries for all users in future versions (e.g. switching from ponyfill "A" to ponyfill "B").

FWIW, docsify-themeable bundles css-vars-ponyfill, but this is only because times were different back when docisfy-themeable was created. If I were doing it today (and this is a change I will likely make in the future), I'd load css-vars-ponyfill via dynamic import instead of bundling:

const hasCSSCustomPropSupport = window?.CSS?.supports?.('(--a: 0)');

if (!hasCSSCustomPropSupport) {
  import('https://cdn.jsdelivr.net/npm/css-vars-ponyfill@2')
    .then((module) => {
      // Do stuff...
    });
}

(If you unfamiliar with the window?.css ... syntax, see @babel/plugin-proposal-optional-chaining for details)

@anikethsaha
Copy link
Member

We need to ship polyfill for dynamic import as well right ?

@jhildenbiddle
Copy link
Member

Nope. Bundlers like Rollup and Webpack handle these without issue or need for a plug-in.

@anikethsaha
Copy link
Member

ahh, yea
We will be migrating to webpack so I guess we can make use of that

@jhildenbiddle
Copy link
Member

What’s the issue with Rollup? Why Webpack vs Parcel, Snowpack, or some other bundler?

Not pushing for one over another, just wondering what the rational is for moving away from the current tool?

@anikethsaha
Copy link
Member

webpack is kind of optimal for creating a browser bundle.
We will be using rollup for the cli

@trusktr
Copy link
Member

trusktr commented Jun 3, 2020 via email

@trusktr
Copy link
Member

trusktr commented Jun 3, 2020

As an idea, here's what Tween.js' output dist/ folder looks like: https://github.com/tweenjs/tween.js/tree/1f85d6e74491037653ddad2ee4193958f09928e2/dist

Probably not many people (if any?) will use the .amd.js build. Modern bundlers, and native ES Modules in browsers, will make use of the .esm.js output. The .umd.js output is what people use for the traditional global-style <script src=""> tags.

@trusktr trusktr changed the title Theme Color IE issue Theme Color IE issue (improve bundling, etc, for older browsers) Jun 3, 2020
@jhildenbiddle
Copy link
Member

Let's move the bundler conversation over to #1168.

@jhildenbiddle
Copy link
Member

Fixed in #1743

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants