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

Bundle doesn't seem fully side-effect free #3

Open
keul opened this issue Jun 24, 2021 · 5 comments
Open

Bundle doesn't seem fully side-effect free #3

keul opened this issue Jun 24, 2021 · 5 comments

Comments

@keul
Copy link

keul commented Jun 24, 2021

(moving discussion there to not annoying people in the Leaflet/Leaflet#7055).


I finally fixed every plugin and packages of my bundle using @ec-nordbund/leaflet.

Everything was working properly in development mode, but as soon as I moved to production I found a new issue.

Long story short: the leaflet bundle is marked as side-effects free but probably isn't.

In my code I have these lines:

import {
  Icon,
} from 'leaflet';

Icon.Default.imagePath = 'https://unpkg.com/[email protected]/dist/images/';

Icon.Default.prototype.options = DEFAULT_ICON_OPT;

The meaning of these lines are related to a SO question about changing the default icon.

When I build my code for production (mode='production' in webpack mode) any reference to Icon.Default is undefined.

So I spent some time trying to identify which change from development to production mode introduced the issue.

If I switch-off optimization.sideEffects in my production build (this flag is automatically put to true in production mode) the issue disappear but obviously I loose every benefit from using the custom Leaflet bundle.

BTW: Not 100% this is an issue of the custom bundle

@mathe42
Copy link
Member

mathe42 commented Jun 24, 2021

Yes you're right thought that this should be ok... As all sideeffects are isolated for that file. But looks like I'm wrong will fix this.

You can try @ec-nordbund/leaflet-gesturehandling this is the automated version of my plugin builder - but it might not work.

It exports NonTiledLayer, TimeDimension and timeDimension. No CSS included.

see Branch https://github.com/EC-Nordbund/Leaflet-Builder/tree/plugins

@mathe42 mathe42 mentioned this issue Jun 24, 2021
@mathe42
Copy link
Member

mathe42 commented Jun 24, 2021

If I switch-off optimization.sideEffects in my production build (this flag is automatically put to true in production mode) the issue disappear but obviously I loose every benefit from using the custom Leaflet bundle.

No you get full TreeShaking! As we don't have window.L.

@keul
Copy link
Author

keul commented Jun 24, 2021

If I switch-off optimization.sideEffects in my production build (this flag is automatically put to true in production mode) the issue disappear but obviously I loose every benefit from using the custom Leaflet bundle.

No you get full TreeShaking! As we don't have window.L.

Unluckily putting optimization.sideEffects to false in my env makes Leaflet chunk size back to old full size. Also: this is a global settings so it will acts also on other dependencies too.

BTW: I was able to fix the issue above by using:

 import {
   IconDefault,
 } from '@ec-nordbund/leaflet/src/layer/marker/Icon.Default';
 Icon.Default = IconDefault;

But then another issue appeared (this time about the CRS).

Probably my Leaflet env is too complex. In any case: it worthed a try!

Thanks!

@mathe42
Copy link
Member

mathe42 commented Jun 24, 2021

Oh I found the problems:

For example:

Map.addInitHook('addHandler', 'boxZoom', BoxZoom);

We can say there are Sideeffect free as that is only needed when we use that part of leaflet. Only some problems (lice the Icons) will appear.

When you provide a full code snippet like your above I will add it to the bundle so all needed code is loaded.

The more I read the leaflet code I want to rewrite it for modern Browsers (, with typescript) and better code style that is treeshakeable... The hole not useing class syntax is really bad and what has to run today in IE10 or below?

Just thinking:
Maybe I can remove all Map.addInitHook calls or wrap them in functions that you have to call... But I think that is a bad solution...

[EDIT]: Just tinking I can collect all the Map.addInitHook calls and move them to a extra function that a user can call so all is loaded but if not called treeshaking is done...

@keul
Copy link
Author

keul commented Jun 25, 2021

Yeah, I hope to see a fully ES6+ Leaflet 2.0 someday 🤷‍♂️

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

No branches or pull requests

2 participants