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

Better Pre-shaking support (multi-entry) #365

Open
jaredpalmer opened this issue Dec 6, 2019 · 13 comments
Open

Better Pre-shaking support (multi-entry) #365

jaredpalmer opened this issue Dec 6, 2019 · 13 comments
Labels
kind: feature New feature or request topic: multi-entry Related to multi-entry support

Comments

@jaredpalmer
Copy link
Owner

jaredpalmer commented Dec 6, 2019

Tree shaking is fragile, it would be nice to support preshaking more easily in cjs and esm. Right now, because of how we handle the cjs entry point using the name flag, you would have to run tsdx once for each entry to get Dev / Prod behavior. Maybe this is fine, but maybe there is a cleaner way with tsdx config or cli flags. One idea would be to pass multiple names that are comma separated and then map to each entry file. However in big ui libs, that wouldnot scale well, so maybe we could take a glob pattern?

I also think that we might want to automate the creation of the root level export files. All these .js files do is export the correct dist file. This is required though for require(‘formik/Field’) instead of require(‘formik/dist/Field’). We also may want to update ensure that these root files are correctly list in package.json files array

Thoughts?

@jaredpalmer jaredpalmer changed the title Pre-shaking (multi-entry) Better Pre-shaking support (multi-entry) Dec 6, 2019
@jaredpalmer jaredpalmer added the kind: feature New feature or request label Dec 6, 2019
@swyxio
Copy link
Collaborator

swyxio commented Dec 6, 2019

I also think that we might want to automate the creation of the root level export files.

yea this is clearly a highly requested feature. i'm interested in convention over configuration for this one. once i can require formik/Field i'd be easily surprised if i couldn't also import formik/Field/SubField. this would be no problem at all in jsland, would be nice to support in tsdx with no further change in contract.

maybe the solve here is src (and potentially types) are disallowed folder names inside src, and tsdx is free to compile src out to the root level for publish (may want to handle the gitignore too). this would work for esm but not sure how this looks for cjs.

@JustFly1984
Copy link

JustFly1984 commented Dec 7, 2019

@sw-yx I'm looking at bundlephobia https://bundlephobia.com/result?p=@react-google-maps/api@1.8.0 at my package built with tsdx, and I see that there is 14.2kb gzipped total, and each export is 14.1kb, so clearly there is no pre-tree-shaking here. before with webpack and babel I could build each component in separate file, and later import each component from it's ./lib/ production file. and so it would take even less code to be added to the final product.

@jaredpalmer
Copy link
Owner Author

@sw-yx +1 for convention. As for entries, yeah we could restrict src, test, types, etc.

@Andarist
Copy link

Andarist commented Dec 8, 2019

I'm not a user of TSDX myself - but I'm kinda into optimizing libraries structure etc and have thought & used different strategies in the past. I think what https://github.com/preconstruct/preconstruct does currently is the sweat spot for multi-entries. There is a config like this:
https://github.com/emotion-js/emotion/blob/8b59959f0929799f050089b05cafb39ca2c57d2d/packages/styled/package.json#L53-L59
and the preconstruct tool reads it and generates appropriate rollup config with multiple inputs (it works by convention - entry name relates to src/{{entry}}) and for each entry it creates such directory:
https://unpkg.com/browse/@emotion/styled@11.0.0-next.8/base/
where ofc rollup is configured to output entry files to that nested dist directory.

Locations etc cannot be configured - everything works by convention and preconstruct validates values in those nested package.json files etc + offer to fix them if they dont follow the convention.

@dbismut
Copy link

dbismut commented Dec 23, 2019

Hey, I'm maintaining React-use-gesture and have been using tsdx since v6. v7 is now in beta and each export should be side-effect free, but bundlephobia shows all exports being the size of the full library.

I'm not sure I understand the gist of tree-shaking, but I was wondering if this was related to @jaredpalmer first post?

@Andarist
Copy link

@dbismut I've analyzed your bundle and it seems there is nothing wrong with it (at least not in a way that would result in full size to be reported for each individual export). It seems that this is a bug in bundlephobia.

@dbismut
Copy link

dbismut commented Dec 23, 2019

I guess I’ll check with them directly thanks @Andarist

@jaredpalmer
Copy link
Owner Author

might be too restrictive, but why not just look at the files array in pkg.json? Aside from dist, could we presume if any other item in the array has a matching sister .ts or .tsx file in src that we should output a top level .js file? As an mvp, I’m not sure we need to deal with folders even, we can add later. Fundamentally the files array is the source of truth anyways. Thoughts ?

@Andarist
Copy link

  1. wouldn't work for people using .npmignore or no filtering at all
  2. wouldn't produce dual CJS/ESM files and thus for packages sharing code between "main" entry and extra ones (not that uncommon) it would actually increase the consumed bundle size, because some duplicated would get in (if importing, supposedly, shared code through multiple entries)

@jaredpalmer
Copy link
Owner Author

@Andarist what's your suggestion then?

@Andarist
Copy link

Andarist commented Jan 1, 2020

I think "proxy directories" are the only way to go - with a single config producing a single file output set for given entries. What I mean - if you output esm you should do that with a single rollup config, otherwise you might end up duplicating code.

That's the easy part, as building such config for rollup is rather trivial. Problem is - how to express intent for multi-entries in tsdx configuration (and if you should output those proxy directories automatically or not).

From what I see tsdx is mostly 0-config, but it has some configuration as CLI options? I see it being used for example here:

opts.input = await getInputs(opts.entry, appPackageJson.source);

I would advise creating basic support for config files (maybe just package.json key? although not personally I'm not a fan of configs in package.json), because it really seems easier to work with than a long list of CLI args.

With that in place, I would start with adding support for simple case - no globs, nothing fancy, just an exact list of desired output entries. A convention that they have to match exactly to files inside src is IMHO a good one and could be used here.

With such a list you could generate those extra directories for a user with appropriate package.json files inside them. I would only be extra careful here - you shouldn't just try to overwrite existing files and if you find existing stuff you should validate their content against expected shape. It shouldn't be an exact match, but rather you should just validate main/module (and other fields if necessary).

I wouldn't be concerned with cleaning those directories, let them be committed - it doesn't really matter. Validating a package setting (looking up package.json#files and .npmignore) if they are going to be published with a package would definitely be a nice touch.

@agilgur5
Copy link
Collaborator

@Andarist would be great if you could take a look at #367 , which supports multiple entries, including globs (which were added, but non-functional, in #175 ), and considers various trade-offs mentioned here.
You can create, e.g. component/package.json and add it to files on top of that PR to support root-level entries with multiple formats, which I'm doing in one of my libraries. Automatically creating files, while helpful, is quite opinionated & advanced, in ways that may confuse authors as too much control is being taken away during the publish process, so I'm currently opting to manually create root-level entries instead.


for packages sharing code between "main" entry and extra ones (not that uncommon) it would actually increase the consumed bundle size, because some duplicated would get in (if importing, supposedly, shared code through multiple entries)

This is one issue I'm not sure how to handle with Rollup. Rollup has code-splitting, but it creates async split-points, whereas just sharing code between entries could be done with ordinary sync modules/split-points.
preserveModules allows for this and is also a separate feature that I believe should be supported (e.g. #382 ), but not bundling anything isn't ideal for all packages

I think both re-using code between entry bundles and preserveModules are separate features that can be added on top of #367 IMO. #367 is more of the "bare support" upon which more features or opinionated structures can be added. Code re-use, preserveModules, and auto files are also not necessarily ideal or necessary for all packages

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 7, 2020

Just an update here that I've figured out how to do code-splitting in #367 (which still hasn't been reviewed or merged). And I've added a separate PR for preserveModules at #535 .

Auto files for multi-entry is still up in the air, but you can see an example of how to manually do multiple package.json for each entry with #367 in my very tiny library window-resizeto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New feature or request topic: multi-entry Related to multi-entry support
Projects
None yet
Development

No branches or pull requests

6 participants