-
Notifications
You must be signed in to change notification settings - Fork 507
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
(fix): multiple entries should output multiple bundles #367
base: master
Are you sure you want to change the base?
Conversation
this is awesome! a big internal change, so would leave to Jared. |
24a6a2d
to
3967c30
Compare
Added another test for subdirs for completion. I think I figured out the reason for the duplicate type declarations -- I also experimented with some {
"main": "./bar.js",
"module": "./bar.esm.js",
"typings": "./bar.d.ts"
} at Creating root-level stubs, a la #365 , does work (so long as they are added to your {
"main": "../dist/foo/bar.js",
"module": "../dist/foo/bar.esm.js",
"typings": "../dist/foo/bar.d.ts"
} at I'm not sure how one would auto-generate these stubs instead, without causing potential conflicts with user files and without having the user to change
|
32cf815
to
03fb221
Compare
Ok, fixed the duplicated type declarations in the latest commit. That's actually not specific to this PR (only the fix part in the commit message is), so could be extracted out as a separate optimization. While testing a bunch of stuff, I noticed that sometimes |
@agilgur5 that's a BAD bug if it's true. oof. I think this is dope, I def want to play with it and kick the tires to think through some of the edge cases. I don't actually think nested directory stuff matters that much. We could mandate that entry points need to be at the top level. |
33d634d
to
b8f0f4b
Compare
ecafed2
to
4466bb9
Compare
Ok, refactored a bunch so now there's no real issues with subdirs or Also changed some of the And added a test for globs because apparently globs are already supported for multi-entry, they just expanded and hit this same bug before this fix. Think this is mostly good to go now, have ironed out most of the things I was unsure of. The I'd also like to refactor a bit once this is approved as
Yeaaa, I haven't encountered it again, so hopefully that was just some fluke (or, y'know, filesystems not being reliable).
Well nested directories already work haha 😅 . I do think they are useful however, @sw-yx even pointed out a Formik use case in #365 (comment) and something like #321 could be replaced with a |
8751eb3
to
a70469f
Compare
Ok, weird, Windows CI errors started happening once I rebased with master (to fix merge conflicts). Not totally sure why they started now or what the error is there 😕 |
Sorry did not mean to close |
I'm now wondering if just re-running |
1ec46b1
to
3401e51
Compare
@jaredpalmer it might be good to have anyway, but for multi-entry specifically, I listed a few of the issues / cons with re-running
If we refactor parts of TSDX into Rollup plugins, we might be able to re-use Rollup's internal multi-entry support (pass input and output as arrays) and that might be a notable optimization / speed boost (though idk how Rollup's internals work, so maybe not) |
I've been using @agilgur5 's fork |
@kantorcodes anything special you need to do to get it working ? I cant seem to make it work properly. |
I was able to apply the changes of this PR to my project with patch-package. Seems to be working well for my case. Hope it gets merged soon! |
Changes copied from jaredpalmer/tsdx#367.
Im looking forward for this release. Would be a great addition 😄 Is this even on the roadmap for release? or is there any type of timeframe in mind as to when this release might go out? |
Is there any way to help move this forward? The list of exports of |
@phryneas I've made several extensive comments about what is leftover that have never been responded to by anyone. To repeat what I've written a few times: Problems
Noteworthy HistoryThis was one of my very first PRs to TSDX as a contributor almost a year ago and it has never once been reviewed by a maintainer (same with some of my other PRs from that time...), which, as was written, caused it to need significant rebasing over time -- it'd be difficult for me to estimate how many hours were spent on this single PR. That being said, the problems still exist, but this PR could be released with some incomplete or poor DX as -- at least as far as I know as no one has really responded to the contrary -- there seem to be no good solutions to those problems and I know many users, myself included, are using my fork/various branches (I am not sure how well, that being said). |
I'm sorry. Github did this best to collapse the important posts, so this issue looked primarily like a long list of "when will this be released?". Went through the posts now, but still did not understand everything - I might still be missing some review comments.
I'm sorry, I'm not familiar with that term and it only seems to be used in this issue. I imagine a "proxy directory" would mean an
I'd honestly be fine to be able to just specify what entry points go into one UMD outpt. So a library could decide if it wanted to ship a combination of all entry points, or omit some from the UMD build.
Yup, you're right about that.
I know these PRs just too well - and also how important a real life is. ^^ That said, after your comment, I did a more thorough read. Given all you pointed out, I imagine even if you released this now, it would probably not match the use case I had in mind. And tbh, I'm not sure if that wouldn't be asking too much of tsdx in general. I tried to write some suggestions, but everything I can suggest would take tsdx away from the great zero-config build tool to just another semi-abstraction over a rollup config - which would probably benefit noone in the long run. So I'll probably try to experiment with some manually written rollup config. Sorry for causing a stir :) |
Not quite - it's a directory that lets different tools to pick up the best format of the output bundle. For the "root" of the package this is handled by the root's package.json and the same principle goes for nested "entries". This is an example of such a thing in the wild:
I don't really want to steal anyone's thunder here or anything - just mentioning an alternative that already works quite great and provides a great DX from my PoV: https://github.com/preconstruct/preconstruct . Its job is pretty much the same as tsdx - it is just more opinionated about some stuff, but that has its benefits because it can actually validate much more (and even propose autofixing). |
Yes ^that. I've linked a much smaller example a few times in my previous comments, my own
@phryneas that's quite a large ask since those are effectively different builds. If I'm remembering correctly, the simplest solution for UMD is to just include all the entries as one bundle. That way there are no extra flags. But what you're proposing requires adding at least one if not more flags that'd have to be input multiple times -- that would be a very complicated and hard-to-understand API, as you seem to later point out. Given that UMD doesn't have the highest usage with TSDX users nor with consumers (on top of things like performance penalties), and that browser ESM is widely supported now, I'm not quite apt to putting in significant time for supporting lots of complicated, UMD specific features.
Other than that being easier said than done, this PR exists for a reason. I would like to support multi-entry officially and both myself and others here are using unofficial support provided by this PR/my fork/branches.
@Andarist I've seen you mention
And then didn't respond when I more explicitly pointed out the intrusiveness of generation and the lack of backward-compat with conditional exports (and their instability at the time) 😕 Validation and proposing auto-fixes is a good feature though, and the validation piece I've been considering to build as a separate library that TSDX could use (I think it's quite useful to authors more broadly) since #494 which points out some other common package issues. All of these can be done as separate, incremental features, however, and don't quite solve the proxy directory problem, but you had explicitly said you were against releasing this without auto-generating proxy directories... 😕 |
I haven't tried this branch, but in general I'd be happy to get something merged, and then improve DX separately. It could be marked as "beta" or something to indicate the DX doesn't match your ideal standard yet. |
I was trying this out on a new library I was working on setting up for this configuration which will have the following setup (well, the relevant bits): /src
index.tsx # Entry point
/primitives
index.tsx # Entry point
package.json # Edited to include the other root files in the files array
primitives.js # points at /dist/primitives to allow consumers to import as `import {xyz} from 'package/primitives'` In doing that things mostly work. I ran into an issue where the
Like I said, there might be a better way to do that, but that's the solution that worked for me. |
I haven't really found anything lacking from preconstruct when it comes to TS support (I use it in several TS repositories)
Not quite, you only have to add an extry point in the config (like here) and preconstruct asks you:
This can be validated (at least to some extent) - if you have the knowledge about what files are generated and you can grab But I don't feel comfortable discussing preconstruct further here (seems out of place and like I'm trying to pouch users or smth - which i dont) - if u want to discuss more about this or anything else, I'm always available on Twitter and my DMs are open as well :)
Sorry for that, I'm totally slammed with my GitHub notifications :/ so there is a high chance that I've just missed your comment back then. The PR itself also shouldn't be stalled because of my comment by any means - I'm only an external observer here. I got the impression that this is mostly not moving forward because there are not many comments from actual maintainers. As to conditional exports - I agree now that those shouldn't be used as a configuration, they just handle try to handle too much stuff to be easily manipulated by humans (if u really want to optimize them with things like prod/dev bundles, environment specific bundles or |
Well guys, I know you're busy, but this issue it's been here already for a year. From my experience, and I'm already old using tsdx, I've had to start using my own rollup configs and so on because of the growing size of my libraries. As we speak, my core library which is perfectly tree-shakeable weights 160kb, most of which is dead code not used on every page of my website. The same for my UI library which I had to rework myself waiting on this release to be "released". I'm not putting venom on your work, I just want to signal that you actually "sell" tsdx as a tree-shakeable library which in fact does not tree-shake :D . For now I'll use my own config , but I'll be glad to use yours once it will be ready, and I promise I will head others to tsdx as soon as this issue will be solved. (once and forever??) Thank you guys, |
@ibraelillo maybe you should take a stab at fixing it or put a bug bounty on it? :) |
@KATT maybe I should!! haha, and probably I will, if you continue to stab my whole enterprise monorepo with non tree-shakeable code :). I'm already checking what @agilgur5 was already doing on his branch, I've seen some tasks TBD to complete the milestone. I'll try to find some time on the next 2 weeks to see how contribute. |
This option is the only way to keep libraries organized at scale, and I think it should be merged ASAP. Currently, you can either define everything in With multiple entries:
Outputs:
|
Any luck on this? |
Use preconstruct |
Summary
previously, while rollup was passed multiple entries, they all had
the same output.file, and so only the first ended up being output
just said "entry file", which needs some disambiguation now)
adds various tests for simple use case, subdirectories, and globs
documents usage of all those use cases
optimizes emitting type declarations and fixes extraneous typedefs for subdirs
Related Issues
Fixes #16 and fixes #28 . While multiple entries has been "supported" for almost a year now (since v0.3.0 per those issues), the implementation has some issues as documented in #175, and this fixes those to properly handle multiple entries
Fixes #175
although I don't believeEDIT:rollup
supports globs natively, iftsdx
were to support entry globs it would have to implement some handling itself or use a plugintsdx
already supports globs for multi-entry (this was part of the original multi-entry implementation)Related to #365 , which could add more automation for root entry files on top of this.
Adding additional
package.json
s to supportmodule
fields (and others) for each entry file could also be built on top of this (per #175 (comment))Things to Review
IncreateRollupConfig.ts
,opts.name
would never be set tosafeVariableName(opts.name)
since it would always exist as there is a default set innormalizeOpts
(here's the commit that broke it). I changed it to always usesafeVariableName
so that UMD names for subdirectories don't have slashes in them. That's potentially breaking, though really shouldn't be. That's not to say the auto-generated names are good though.output.file
andname
now, but I think that's better than usingname
for both./
replaced with__
. They're notsafe
'd because I believe that would remove the underscores, but they are existing filenames, so they might be safe by definition? But they're not camelCased.name
isn'tsafe
'd, this is a bug per above (|| safeVariableName
was never reached after this commit). Maybe we should fix this in the future (or in this PR)?Subdirectories that havesrc/
in them will cause bugs, becausesrc/
is stripped from the name. Could change it to only strip the firstsrc/
, but that would still be problematic for a custom entry directory like:lib/foo/src/bar.ts
-->dist/foo/bar.js
src/
or./src/
.Minor things to possibly Review
ThetestEntryOutput
is weird and makesjest
failure output harder to read (have to read up one stack). I tried usingit.each
instead, but then theafterEach
teardown hits in between each one 😕 Not sure if there's a better way of doing thisshell.ls
to get an array of output files and then.toContain
so it's explicit which file is missing. Then usedtoMatchObject
so it gives a list of which objects are missing from the output in super nice readable output. Should probably usetoMatchObject
instead of all the.toBeTruthy
tests for its superior readability (not sure about thetoBeFalsy
ones though)Alternatives Considered
Rollup hasoutput.entryFileNames
that could be used in conjunction withinput
objects for mapping of input filename to output filename, but that would require a lot of refactoring, especially because the CJS entry files (i.e.the ones that split b/t dev and prod builds) for each entry would still need to be output.Allowing users to specify multiple
--name
and requiring that for multiple entries. That can be added on top of this as a config option, but the logic here is still required for parsing out filepaths. It could be more an alternative if:Specify
output.file
, at least partially, innormalizeOpts
orcreateBuildConfigs
instead of increateRollupConfig
. This is a very reasonable alternative. We'd still need to figure out whatoutput.name
should be though, so in conjunction with specifying multiple--name
it would work.name
and so that the code doesn't usename
as if it's a proper filepathAllow configuring the
package.json
source
field with an arraymicrobundle
's issue for multi-entry support: Support for multiple "entries" developit/microbundle#50Literally just running
tsdx build
once for each entry file. Honestly, it's not a terrible option and means a lot less code complexity, but besides the noisy output, users would also have to make sure to pass--noClean
and would have to specify a--name
for every other entry fileindex.js
would be overwritten for any entries in the same directory because the nameindex.js
is hard-coded in (until this PR).Caveats
Per #365 (comment), this doesn't currently share code between each entry point. They create separate bundles. I've filed rollup/rollup#3422 to see if if this is even possible.Either way, getting them to share code would require even more significant refactoring (could potentially use
rollup-plugin-multi-input
to help though there's some custom glob resolution already used internally) and may be breaking ifoutput.dir
needs to be used (it's possible that withoutput.entryFileNames
breakage could be avoided)EDIT: Thanks to some help in rollup/rollup#3422, I figured out how to code-split with some minor changes to the code here.
Now the only caveat is that multi-entry for UMD isn't really supported, but you can't really do multi-entry with UMD. That's probably saved as a separate case where one could use
tsdx build --noClean --format umd
because it's a big exception. It's usually a separate case in Rollup guides as well. Though I've followed up on that issue to see the possibilities.