Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

fix: remove unnecessary css and remove esm from package.json #55

Closed
wants to merge 1 commit into from

Conversation

kristw
Copy link
Collaborator

@kristw kristw commented Apr 10, 2019

🐛 Bug Fix

related to apache/superset#7058

Demo

Before

https://superset-ui-plugins.netlify.com

After

https://deploy-preview-55--superset-ui-plugins.netlify.com

Explanation

The way the packages are transpiled now produces two types of output:

  • basic js output in lib directory.
  • es6 module in esm directory.

In package.json

  • field main point to the main entry of the library, which is lib/index.js
  • field module point to the main entry point of the library that is es6 module, which is esm/index.js. This is useful for bundler that are es6 module aware such as webpack >= 2, so it will use the code in esm instead of lib which it can do treeshaking, etc. better.

However, for legacy modules that import css into js file, this creates problems when bundling for production.

  • The code in lib after transpiled will be require('xxx.css') which works correctly with webpack production bundling.
  • The code in esm after transpiled will be import 'xxx.css' which makes the css file not being pulled into production bundle and results in missing css, such as black screen, weird fonts, as reported in the issue above.

Solution

In the longer term, we may need to get rid of the js code that imports css in the library. Either by converting to css in js or create single css file that consumer of these packages have to import by themselves alongside the js, similar to how bootstrap has js and css.

For the short term solution, this PR,

  • removes the small css files that can be converted to just js.
  • remove the module field from package.json of the packages that has import 'xxx.css' statements, so the consumer app/package will be using lib/index.js instead of esm/index.js when webpack resolves the packages. This changes should require the smallest changes from the consumer app. Only bumping the version number.

@kristw kristw requested a review from a team as a code owner April 10, 2019 21:28
@williaster
Copy link
Contributor

Hmm, it seems like a bummer to nix all of the benefits of the esm build and tree shaking.

Can you tell if the problem comes from code here importing .css files? If so, an alternative solution is to remove those imports and instead have consumers of the plugin to import the .css file themselves from lib/ or esm/. I've seen several libs suggest this (like react-virtualized etc.). That way the bundling happens with their bundler (webpack 4+ in Superset), tree shaking can happen, and things should just work 🤞

@mistercrunch
Copy link
Contributor

This goes beyond my grasp of what I know around bundling. I had to Google the term tree shaking, love it :)

@kristw
Copy link
Collaborator Author

kristw commented Apr 12, 2019

@williaster

  • I have talked to Miles the other day and thought in the future we should avoid importing css in libary js code. I have implemented fix: extract css per package to style.css #56 that concatenate every css files in each package into lib/style.css. The result from this PR is the consumer of these libraries will have to import css manually, both style.css and any css dependencies such as mapbox-gl, datatables or nvd3. However, it also does not solve the original problem. So I am hesitating a bit since it is now more cumbersome to import the plugins.

  • The root cause ends up being the sideEffects: true that is set in each package.json. webpack in production mode uses this to determine if the package has any side effects and when it is declared false, it (via treeshaking) drops the file that does not export anything, including css. Treeshaking only applies to esm so when importing lib it works fine. Setting sideEffects: ["*.css"] seems to solve the problem. Just create fix: list css as side effects #57 fixing only the sideEffects.

@kristw
Copy link
Collaborator Author

kristw commented Apr 12, 2019

Closing this one in favor of #57, which is a subset of #56.

@kristw kristw closed this Apr 12, 2019
@kristw kristw deleted the kristw--css branch April 12, 2019 17:12
nytai pushed a commit to preset-io/superset-ui-plugins that referenced this pull request Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants