Reduce bundle size of apps that import @turf/helpers

[smallsaucepan]

As part of a previous bug fix (#2530), unmaintained code from a third party package was included in turf-helpers for use by other packages. This was a convenient approach at the time given there wasn't a clear strategy for where to put code we want to share internally, but not necessary publish.

Unfortunately that lead to a 50kB increase in bundle size by users of turf-helpers, even if they used none of the recently added code.

This PR reverts turf-helpers to its original responsibilities, and instead leverages a recently created third party package that (surprise!) contains exactly the functionality we need.

This approach isn't perfect - the bundle size is still about 4kB larger than it used to be. However this may have more to do with other changes between 6.5.0 and 7.0.0, and will require further investigation down the track.

Resolves #2616.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Have read How To Contribute.
• Run npm test at the sub modules where changes have occurred.
• Run npm run lint to ensure code style at the turf module level.

[smallsaucepan]

Pretty happy with the removal of the code from turf-helpers/lib/ and introduction of geojson-equality-ts.

Would like a proper eye cast over the /* @__PURE__ */ annotations though.

Some functions in helpers couldn't be marked pure as they rely on something outside that function. For example, can't mark geometry() as pure because it depends on point() and lineString() and what happens if those have been tree shaken?

Same for radiansToLength() because it refers to the external factors[] constants array.

Please double check to make sure I haven't missed any. And if you think there is a case where it could be pure please let me know.

[mfedderly]

I'm on board with the stuff that isn't the @__PURE__ annotations. I'm hoping we can avoid that by just moving the code into individual files.

I made that change here #2625 if you have the ability to see if that treeshakes better? I didn't do the geojson-equality removal in that PR, so some amount of stacking with your change is probably required.

Additionally, I just looked at deep-equal today and realized that it brings in a polyfill for sticky regexes, and the developer seems intent on keeping that in (at least 3 years ago). Perhaps we want to try fast-deep-equal instead for geojson-equality-ts? I've had good luck with it in the past.

[VIKTORVAV99]

I'm on board with the stuff that isn't the @__PURE__ annotations. I'm hoping we can avoid that by just moving the code into individual files.

I made that change here #2625 if you have the ability to see if that treeshakes better? I didn't do the geojson-equality removal in that PR, so some amount of stacking with your change is probably required.

Additionally, I just looked at deep-equal today and realized that it brings in a polyfill for sticky regexes, and the developer seems intent on keeping that in (at least 3 years ago). Perhaps we want to try fast-deep-equal instead for geojson-equality-ts? I've had good luck with it in the past.

Id be happy to help test the tree shaking parts later today.

[smallsaucepan]

Thanks for the alternative @mfedderly. Did a test with no deep-equal, no PURE comments, and then contents of #2625 merged. These were the results in library mode:

✓ 6 modules transformed.
dist/my-lib.js 8.03 kB │ gzip: 3.01 kB
dist/my-lib.umd.cjs 6.89 kB │ gzip: 2.90 kB

Which is pretty much identical to "removing deep-equal only" from a few days ago:

Turf 7.0.0 with PR, turf-helpers isObject call
✓ 6 modules transformed.
dist/my-lib.js 8.03 kB │ gzip: 3.01 kB
dist/my-lib.umd.cjs 6.89 kB │ gzip: 2.91 kB

I don't think it's helped with the tree shaking.

They're not ideal, however the PURE comments seem to be the most effective, and it sounds like they're widely supported. Is it mainly aesthetics that put you off? They'd only need go in packages where we're exporting multiple things i.e. helpers and maybe meta.

[mfedderly]

I don't think it's helped with the tree shaking.

Aha I looked into this some more. tsup is just bundling it all into a single file anyhow so all my file splitting did exactly 0 to change the tsup output 🤦 . Let me keep digging here.

In the meantime, I have a few concerns with the changes proposed here.

The first is that changing from function to const can cause weird runtime initialization order problems where something defined earlier in the file depends on something being defined later in the file, which isn't the case with const (but is with var or function). I'm not sure if that was actually required to get size savings or not.

The second is that manually adding the PURE annotation seems hard to reason about (can you mark the factors value as PURE even if other things depend on it?) and hard to keep in sync. There may be a way for our build system to just insert them automatically for us.

[mfedderly]

Ok I think I see what's happening here. tsup is combining everything into a single file, and then the keepNames setting adds a __names() call for every class and function that gets exported, where the calls are not marked as @__PURE__ and so it causes the retention of all of the functions.

If the files weren't combined, I think my PR would've helped here as the things could have been treeshaken from the index file. If we removed keepNames, the sideeffectful __name calls don't get put into our output, and therefore do not cause the retention of unused code.

Because we do not minify our output, I think we can remove keepNames and manage to restore treeshaking throughout all of the packages in one action. With keepNames disabled, we still get the original names in the output from our build, they just aren't included as strings. I think if a consumer is doing minification they may want to enable keepNames, but it doesn't seem necessary for our builds.

Thoughts?

[smallsaucepan]

The first is that changing from function to const can cause weird runtime initialization order problems where something defined earlier in the file depends on something being defined later in the file, which isn't the case with const (but is with var or function).

This wouldn't be relevant in our case as we're not invoking any of the functions during importation / execution of the helpers file.

Regardless, this syntax is also supported:

/* @NO_SIDE_EFFECTS */ export function f() {}

The second is that manually adding the PURE annotation seems hard to reason about (can you mark the factors value as PURE even if other things depend on it?)

None of this is stating other things don't rely on factors. It's saying, if nothing else we want to keep uses it, it's safe to leave it out of the bundle entirely. It has no impact on the system other than the value it returns - it sets no global or local state, etc.

I took a pretty exacting approach to what pure meant in this first pass as well. In reality we could probably mark the other constants as PURE, and most (all?) the functions as NO_SIDE_EFFECTS.

and hard to keep in sync.

I wouldn't have thought so? It's about a dozen comments in one file. 40 if we end up annotating the rest.

I don't think any of the above are significant enough to eliminate it.

In addition to that, removing keepNames means we are:

• artificially splitting helpers into 40 odd files - it doesn't make sense to keep Units and AreaUnits in separate files. Or radiansToDegrees and radiansToLength, or isNumber and isObject
• breaking readme generation and website generation for the helpers module - both those will need to be refactored
• making a project wide change to our build config - do we know the full impact for all users? Plus, we do actually minify some code i.e. @turf/turf/turf.min.js. Will it affect that?

Feels like annotations are a much more proportional approach to solving this 🤷

[mfedderly]

When I mean hard to keep in sync, I don't disagree that a few comments in @turf/helpers is relatively easy to do today, but keepNames generates code that is not tree-shakeable in every package. I think that @turf/turf remains tree-shakeable, but only because its re-exports do not get inlined, and so tree-shaking has an import step to use.

Disabling keepNames would not require that we split @turf/helpers into many files. If we didn't roll up all of the code into a single file I think the PR splitting it into individual files would have re-enabled treeshaking even with keepNames, but I agree that having all of those files is pretty annoying. I think this also addresses the readme/website generation concerns if we don't need to split out the individual definitions?

Even though its a wider change to disable it, I don't think keepNames is actually making a difference for consumers other than causing us to have to add annotations to fix treeshaking. If you disable it and then look at the output files, as far as I can tell the names are still retained in the same fashion that they were there in the 6.x builds.

turf.min.js is a special case, and has its own build configuration which does not need to specify keepNames, because it is just a rollup. Having built with keepNames off, I can still import it and use it with the normal method names.

[smallsaucepan]

Probably best then to make a call which PR - this one or #2625 - is closer to the result you want, and then push some commits with the combo you want to go with. If removing deep-equal is still part of the strategy, you could cherry pick the first two commits from this PR into 2625 and go from there.

Doc generation assumes index.?s is the only file in each package to parse, so anything different will require some level of refactoring. You might be able to reconfigure documentation.js to follow dependencies (see --shallow) though not sure if that would then document every dependency imported into every turf-*/index.?s

[mfedderly]

What I'm advocating for is this PR minus the changes to @turf/helpers and delete the keepNames from the tsup configuration.

[smallsaucepan]

Have rolled back the annotations, and removed the keepNames config. Double check that reflects what you had in mind, and it should be good to go.

[smallsaucepan]

Sorry @mfedderly, I misread this line:

Disabling keepNames would not require that we split @turf/helpers into many files ...

I took that to mean "not as many, though still multiple" files, hence the comments about docs still breaking. Realise now you were saying would not require splitting at all. Apologies for not grasping what you were getting at 👍

[twelch]

Swapping in the new equality function looks good to me. I trust that dropping keepNames has the intended effect, I don't have the knowledge that Matt does.

[mfedderly]

LGTM also. Do we want to ship a prerelease of this so that @VIKTORVAV99 can test it and give us another confirmation that we've sorted out the treeshaking before doing a full release?

Feel free to just enable the prerelease action, merge this PR, and then disable it if you want.

[smallsaucepan]

Did as suggested with the prepublish action, and have notified Viktor to give it a try.