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

Prevent style sheets from being removed by tree shaking #3542

Merged
merged 1 commit into from
Feb 27, 2022

Conversation

abought
Copy link
Contributor

@abought abought commented Jan 18, 2022

Purpose

Prevent an issue where CSS styles were sometimes stripped out by webpack tree shaking. When performing production builds with optimizations, this caused the table to appear without styling.

These optimizations are turned on (though often only for production mode) in some default Vue CLI or create-react-app configurations.

Details

Tabulator 5 was restructured to use ES6 modules, and its package.json includes a sideEffects: false declaration.

Some applications use a common piece of webpack magic to embed styles via import 'filename.css'. In production (but sometimes not development mode!), all such side-effect-only imports will be stripped out, because Tabulator declares itself to have no side effects.

See active webpack issue discussion for examples of other tools that have been surprised by this behavior. It is particularly annoying for projects that want to use new Tabulator features without upgrading their entire build tool. (since vue cli and create react app manage the webpack config internally)

The easiest fix is for tabulator.js to explicitly declare that CSS and SCSS files have side effects. (see webpack docs). Eg, this is used by bootstrap-vue.

Testing

A specific internal app of ours uses vue-cli to build a component that declares import 'tabulator-tables/dist/css/tabulator_bootstrap4.css';.

(behavior seen for vue CLI 3 projects; not sure about vue CLI 4)

This was tested using npm link for a local copy, before and after this PR change.

Before side effects:
Screen Shot 2022-01-17 at 11 33 43 PM

After:
Screen Shot 2022-01-17 at 11 29 35 PM

Further notes

Our team really appreciates Tabulator! Since this has bitten several of our Vue.js projects recently, I've posted the initial PR for visibility.... but I recognize that the test case depends on specific webpack config details. If you'd like me to create a more elaborate test case for evaluation, please just ask and I can continue adding to this PR over the next few days.

@jflifecycle
Copy link

It is AMAZING that this was just posted because I just ran into this issue when upgrading to v5 from 4.9. This exact problem.

I thought I was losing my mind because after looking at my built css files the word 'tabulator' didn't appear at all. Even though I very clearly was importing the bootstrap4 css file correctly because purposefully introducing a typo caused it to complain the file was missing.

Bless you. Bless us all! Everyone!

@olifolkerd
Copy link
Owner

Hey @abought

You will have to excuse my ignorance on this issue, this was my first attempt at integrating tree shaking capabilities into Tabulator.

When i was initially testing this functionality, i fund that if i did not set sideEffects to false then the javascript was not effectively shaken and the resulting bundled packages contained all of the java script regardless of it being explicitly imported or not.

Have you tested to ensure that this is indeed the case, as i do not want to allow importing of the CSS files at the expense of the ESM treeshaking efficiencies as that would undermine the whole point of the update.

Also how does this account for the multiple themes that Tabulator has available, i would hate to force users to have to include the css for all themes in one go, it would break the layout and waste code.

Cheers

Oli :)

@abought
Copy link
Contributor Author

abought commented Jan 28, 2022

You will have to excuse my ignorance on this issue, this was my first attempt at integrating tree shaking capabilities into Tabulator.

Webpack is a subtle beast; this one surprised me too! In particular, the notion of a boolean flag that accepts three possible forms can be... jarring. (details) This flag doesn't tell it to include css files, but rather, it identifies any included assets as not eligible for automatic tree shaking.

Your questions are very nicely thought out, and quite reasonable. To avoid surprises, I'll try to create a basic test scaffold using a recent vue CLI (known to reproduce this bug) for exact validation- hopefully this weekend, time permitting. A particular focus will be to verify the basic assumption ("a CSS file never imported does not become part of the bundle"). Initial tests looked promising for the CSS, but it's worth a deeper dive for exact numbers.

@olifolkerd olifolkerd merged commit d3746e5 into olifolkerd:master Feb 27, 2022
@olifolkerd
Copy link
Owner

@abought if you are on the tabulator discord server, drop me a private message and i will give you a contributor role so other can see you have contributed to Tabulator

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

Successfully merging this pull request may close these issues.

None yet

3 participants