Mark shared packages as side effects free

[nickrum]

API extensions using Typescript import one of the define*() from @directus/extensions-sdk. Rollup's tree-shaking heuristics aren't prefect. So in order for this import to be properly tree-shaken away by rollup, the package from which the function is imported (@directus/utils) must be marked as side effects free. Due to reasons not clear to me, @directus/constants also needs to be marked as side effects free.

This was also present in the old @directus/shared package, so this PR essentially only reverts to the previous behavior.

[changeset-bot]

🦋 Changeset detected

Latest commit: bb38e4c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 14 packages

Name	Type
@directus/constants	Patch
@directus/utils	Patch
@directus/composables	Patch
@directus/exceptions	Patch
@directus/app	Patch
@directus/api	Patch
create-directus-extension	Patch
@directus/extensions-sdk	Patch
@directus/storage-driver-azure	Patch
@directus/storage-driver-cloudinary	Patch
@directus/storage-driver-gcs	Patch
@directus/storage-driver-local	Patch
@directus/storage-driver-s3	Patch
directus	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[rijkvanzanten]

@nickrum arent most packages of ours side effect free?

[azrikahar]

With additional context from #11329, this PR does seems to resolve the inefficient tree shaking when this sideEffects property was not present. Here are some observed outcome:

Sample file used

import { defineEndpoint } from '@directus/extensions-sdk';

export default defineEndpoint((router) => {
	router.get('/', (_req, res) => res.send('Hello, World!'));
});

Before this PR

• Filesize: 225 kb
• Code: (omitted since it's a tad large, but I can share it if deemed necessary)

After this PR

• Filesize: 1 kb
• Code:

  "use strict";require("axios");var e=e=>{e.get("/",((e,r)=>r.send("Hello, World!")))};module.exports=e;

---

That being said, @nickrum as seen by the require('axios') in the "After this PR" result, do we want to (or can we?) mark the composables package as sideEffects free as well? As the axios seems to be included due to use-items composable if I'm not mistaken, but it shouldn't be present in this case. A quick test I did by adding sideEffects: false to the composable package seems to make sure the built file turns out to be the following:

"use strict";var e=e=>{e.get("/",((e,l)=>l.send("Hello, World!")))};module.exports=e;

but I'm not sure if that's too naive.

[paescuj]

Rollup's tree-shaking heuristics aren't prefect. So in order for this import to be properly tree-shaken away by rollup, the package from which the function is imported (@directus/utils) must be marked as side effects free. Due to reasons not clear to me, @directus/constants also needs to be marked as side effects free.

I'm pretty sure this is due to the index.ts files where all the "re-exports" happen (see e.g. in constants) since these make it impossible for Rollup to detect if the imported modules are side effect free.

I think the solution here might be to bundle these packages themselves as well, but we could look at that in a later step.

@nickrum arent most packages of ours side effect free?

I'm pretty sure they are and for the moment should be declared as such. For example the composables package as mentioned by @azrikahar

[rijkvanzanten]

@paescuj thoughts on removing those index files? DX is a tad less nice, but at the same time it makes it super obvious where imports are coming from so maybe it's a win-win?

[paescuj]

@paescuj thoughts on removing those index files? DX is a tad less nice, but at the same time it makes it super obvious where imports are coming from so maybe it's a win-win?

Thought about this as well, but I'm a little put off by the idea of having to update all imports every time we rename or relocate a file (may not happen often, but is definitely a possibility). Also don't know if we really need this, but at the moment we have the control over which modules we actually want to export. If we want to keep this, we would have to explicitly specify all modules to export in the export field of the respective package.json files.

[rijkvanzanten]

@paescuj thoughts on removing those index files? DX is a tad less nice, but at the same time it makes it super obvious where imports are coming from so maybe it's a win-win?

Thought about this as well, but I'm a little put off by the idea of having to update all imports every time we rename or relocate a file (may not happen often, but is definitely a possibility). Also don't know if we really need this, but at the moment we have the control over which modules we actually want to export. If we want to keep this, we would have to explicitly specify all modules to export in the export field of the respective package.json files.

Yeah now that I'm awake and thinking about this better it's not the right solution. We'd still want people to be able to do things like import { x, y, z } from '@directus/example';, which would require that single entrypoint. That being said, I believe everything is side-effects free

[paescuj]

@paescuj thoughts on removing those index files? DX is a tad less nice, but at the same time it makes it super obvious where imports are coming from so maybe it's a win-win?

Thought about this as well, but I'm a little put off by the idea of having to update all imports every time we rename or relocate a file (may not happen often, but is definitely a possibility). Also don't know if we really need this, but at the moment we have the control over which modules we actually want to export. If we want to keep this, we would have to explicitly specify all modules to export in the export field of the respective package.json files.

Yeah now that I'm awake and thinking about this better it's not the right solution. We'd still want people to be able to do things like import { x, y, z } from '@directus/example';, which would require that single entrypoint. That being said, I believe everything is side-effects free

Yes, I think so too 👍 For now, let's additionally mark @directus/composables as side-effect free and then merge like this.

Edit: And as correctly pointed out by @br41nslug also @directus/execeptions ❤️

[nickrum]

That being said, @nickrum as seen by the require('axios') in the "After this PR" result, do we want to (or can we?) mark the composables package as sideEffects free as well?

That require('axios') was definitely not included when I was testing this. Interesting 🤔
All of the packages which were previously part of shared should be side effects free and can be marked as such (thanks @br41nslug and @paescuj)👍

I'm pretty sure this is due to the index.ts files where all the "re-exports" happen (see e.g. in constants) since these make it impossible for Rollup to detect if the imported modules are side effect free.

Wouldn't that mean that most of the bigger js libraries wouldn't be tree-shakable? AFAIK rollup does treeshaking in multiple passes, one of which specifically includes re-exports, and this little test shows that rollup is perfectly capable of tree-shaking re-exported values.
I suspect this has something to do with zod, which is used in the constants package, as marking every call to zod with /*@__PURE__*/ does seem to make rollup output a clean extension entrypoint.

[paescuj]

Wouldn't that mean that most of the bigger js libraries wouldn't be tree-shakable? AFAIK rollup does treeshaking in multiple passes, one of which specifically includes re-exports, and this little test shows that rollup is perfectly capable of tree-shaking re-exported values.

Yes, I was wondering the same! I just remembered reading that somewhere. Probably the heuristics are much better nowadays. I'm honestly glad if this is not the problem 😄

I suspect this has something to do with zod, which is used in the constants package, as marking every call to zod with /*@__PURE__*/ does seem to make rollup output a clean extension entrypoint.

Yes, this is very likely the case 🕵️ 👍