internal.js coming in way of TreeShaking

[damooo]

Hello guys, Thanks for manifesto..

in latest commit, you created internal.ts as proxy for importing inside lib. now with this, bundlers like rollup cannot perform tree-shaking. let's say we want to import only Canvas class into a project, without entire library, then as that imports exports from internal.js, and it inturn imports everything, then rollup bundles entire manifesto for one class. tree shaking is essential for smaller builds. internal.js giving no other practical value than giving these inconviences and creating circular imports, etc.

[edsilv]

Hmm, internal.ts was necessary to avoid circular imports though: https://medium.com/visual-development/how-to-fix-nasty-circular-dependency-issues-once-and-for-all-in-javascript-typescript-a04c987cf0de

[damooo]

this pattern may be good for applications, which are not depended by others.. but libraries require tree shaking. is circular imports for Type coercion? same author noted in comments:

but to enable tree shaking, you could limit the pattern to those files affected by circular deps (since circulary dependend files will either both or not at all appear in the output). This does make the process more error prone though. But doing that can also be beneficial for code splitting and such.

This might potentially hurt code splitting indeed, the clue is to do it only for parts that are logically bound together (note that you can apply this pattern repeatedly to different parts of your code base!). If done properly, then it shouldn't affect the code splitting

[edsilv]

Yeah I was thinking to maybe only limit it to the affected files. It's the top four here: https://github.com/IIIF-Commons/manifesto/blob/master/src/internal.ts I'm traveling today, so if you wanted to do a PR that would be most welcome :-)

[edsilv]

i.e leave the top four in internal, and remove the rest. The TS compiler will catch the import errors so it should just be a case of adding those back in. Also, index.ts will need to export them too.

[damooo]

I tried to do that.. but tests doesn't passed if we just include only those four in internal.ts.. every time it shows an error for a new module circular import like Thumbnail, Collection, etc.. if i add one by one it is showing remaining in errors, effectively i have to add back most of them to internal.ts. so i restrained from PR.

for size related matters, there is another issue opened #55 .

[stephenwf]

@edsilv @damooo maybe wee need a couple of Types/Interfaces for those classes specifically, allow us to reference their properties without loading them in. I'm not sure exactly how this works, but you can do:

const something: import('./path/to/module').ExportName;

That way it won't actually bundle the module, instead just stripping out the types and ignoring it