tsdx icon indicating copy to clipboard operation
tsdx copied to clipboard

(fix): multiple entries should output multiple bundles

Open agilgur5 opened this issue 5 years ago β€’ 36 comments

Summary

  • previously, while rollup was passed multiple entries, they all had the same output.file, and so only the first ended up being output

    • now, give each entry its own output.file (everything after src/)
      • also handle UMD names by converting subdir '/' to '__'
      • and handle creating multiple CJS entry files
        • clarify that these are CJS in the log output too (before it 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 believe rollup supports globs natively, if tsdx were to support entry globs it would have to implement some handling itself or use a plugin~ EDIT: tsdx already supports globs for multi-entry (this was part of the original multi-entry implementation)

  • also fixes #425, which is a duplicate.

Related to #365 , which could add more automation for root entry files on top of this. Adding additional package.jsons to support module fields (and others) for each entry file could also be built on top of this (per https://github.com/jaredpalmer/tsdx/issues/175#issuecomment-564924731)

Things to Review

  • [x] There are still issues with extraneous type declarations when using subdirectories, as I documented in https://github.com/jaredpalmer/tsdx/issues/175#issuecomment-564814325, but this doesn't seem to be harmful as far as I can tell
    • Fixed.
  • [x] ~In createRollupConfig.ts, opts.name would never be set to safeVariableName(opts.name) since it would always exist as there is a default set in normalizeOpts (here's the commit that broke it). I changed it to always use safeVariableName 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.~
    • I refactored a ton so this shouldn't be an issue anymore. It's a bit more complex code that has output.file and name now, but I think that's better than using name for both.
      • [ ] UMD names for subdirs now have / replaced with __. They're not safe'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.
        • EDIT with code-splitting, UMD doesn't make sense anyway, see below Caveats section
      • [ ] single-entry name isn't safe'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)?
  • [x] ~Subdirectories that have src/ in them will cause bugs, because src/ is stripped from the name. Could change it to only strip the first src/, but that would still be problematic for a custom entry directory like: lib/foo/src/bar.ts --> dist/foo/bar.js~
    • Fixed it to only remove leading src/ or ./src/.

Minor things to possibly Review

  • [ ] String parsing like file extension trimming. Not sure if there's some library already used internally that could just do this with better guarantees.
  • [x] ~The testEntryOutput is weird and makes jest failure output harder to read (have to read up one stack). I tried using it.each instead, but then the afterEach teardown hits in between each one πŸ˜• Not sure if there's a better way of doing this~
    • Updated to use shell.ls to get an array of output files and then .toContain so it's explicit which file is missing. Then used toMatchObject so it gives a list of which objects are missing from the output in super nice readable output. Should probably use toMatchObject instead of all the .toBeTruthy tests for its superior readability (not sure about the toBeFalsy ones though)

Alternatives Considered

  • ~Rollup has output.entryFileNames that could be used in conjunction with input 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.~

    • EDIT: actually used this in conjunction with the changes already made to be able to code-split multi-entry and have correct CJS entry files.
  • 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, in normalizeOpts or createBuildConfigs instead of in createRollupConfig. This is a very reasonable alternative. We'd still need to figure out what output.name should be though, so in conjunction with specifying multiple --name it would work.

    • EDIT: Ended up refactoring to use this so that there are no potentially breaking changes with name and so that the code doesn't use name as if it's a proper filepath
  • Allow configuring the package.json source field with an array

  • microbundle's issue for multi-entry support: https://github.com/developit/microbundle/issues/50

  • Literally 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 file

    • Still then there'd be a bug that the CJS "entry file" index.js would be overwritten for any entries in the same directory because the name index.js is hard-coded in (until this PR).
    • There's also no way of code-splitting with this option

Caveats

~Per https://github.com/jaredpalmer/tsdx/issues/365#issuecomment-576631214, this doesn't currently share code between each entry point. They create separate bundles. I've filed https://github.com/rollup/rollup/issues/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 if output.dir needs to be used (it's possible that with output.entryFileNames breakage could be avoided)~

EDIT: Thanks to some help in https://github.com/rollup/rollup/issues/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.

agilgur5 avatar Dec 12 '19 08:12 agilgur5

this is awesome! a big internal change, so would leave to Jared.

swyxio avatar Dec 12 '19 17:12 swyxio

Added another test for subdirs for completion.

I think I figured out the reason for the duplicate type declarations -- moveTypes() assumes the type declarations are at ./dist/src (hard-coded), and so it doesn't move the types created in subdirectories. This could be fixed by running moveTypes() multiple times on the correct directory, but the type declarations are created for all files on the first pass -- so really we just need to skip emitting declarations after the first entry. If we only emit once, I'm not 100% sure if declarations would still be created for an entry file outside of src/, though that's an unhandled edge case in general.


I also experimented with some package.json auto-generation for module etc fields for multiple entries, i.e. creating something like:

{
  "main": "./bar.js",
  "module": "./bar.esm.js",
  "typings": "./bar.d.ts"
}

at dist/foo/package.json, allowing one to import 'package/dist/foo'. This works when entries are in different directories, but there's no way of doing this for entries in the same directory (since only 1 package.json per directory). Could auto-generate a subdirectory and throw the output files in it with a package.json, but that could be unexpected for a user, and would conflict when there are subdirs named the same as entries.

Creating root-level stubs, a la #365 , does work (so long as they are added to your files array), e.g.:

{
  "main": "../dist/foo/bar.js",
  "module": "../dist/foo/bar.esm.js",
  "typings": "../dist/foo/bar.d.ts"
}

at foo/package.json, allowing one to import 'package/foo'. We could, in theory, detect root-level stubs and use that to auto-configure multiple entries though πŸ€”

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 files on their own. It seems like that would require something like a tsdx publish command that could like:

  1. copy src/ and package.json into dist/
  2. do compilation (into dist/dist)
  3. auto-gen root-entries (dist/foo/package.json)
  4. insert the files field (into dist/package.json)
  5. call npm publish (inside of dist) I'm not sure I'm a fan of the lack of control but I think it would work πŸ€·β€β™‚ Similar issues regarding multiple entries in the same directory. @sw-yx is this what you were thinking of regarding the "convention" case?

agilgur5 avatar Dec 13 '19 02:12 agilgur5

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 .d.ts just randomly don't get generated and one test case will fail. Both times I saw it, all but one .d.ts were produced, so just one was missing for some reason. This was happening before I added the latest commit to only emit declarations once. πŸ€·β€β™‚ that might be a bug in rpt2 (doesn't seem like there's a race condition anywhere), but would be problematic if it appeared just before a library author published 😨

agilgur5 avatar Dec 13 '19 04:12 agilgur5

@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.

jaredpalmer avatar Dec 13 '19 20:12 jaredpalmer

Ok, refactored a bunch so now there's no real issues with subdirs or safe function usage changing. This also makes name not re-used for output.file now, which I think remedies a ton of confusion and edge cases (though makes the code a bit more complex). Updated my initial comment with leftover things to review.

Also changed some of the 'src/' trimming code to only parse out leading 'src/' instead of first 'src/', so checked that off.

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 package.json auto-generation and root-entry generation are very relevant though, so probably good to think through those here too (though that's more feature than bugfix; everything here is for the most part bugfix, but new auto-gen features on top of this may change the usage)

I'd also like to refactor a bit once this is approved as index.ts is getting more and more unwieldy (at the least, extract createBuildConfig into a separate file). It'll likely cause lots of merge conflicts (I mean, even one of my own PRs created a merge conflict on this PR πŸ˜… ) so don't want to do that too early.


that's a BAD bug if it's true. oof.

Yeaaa, I haven't encountered it again, so hopefully that was just some fluke (or, y'know, filesystems not being reliable).

I don't actually think nested directory stuff matters that much. We could mandate that entry points need to be at the top level.

Well nested directories already work haha πŸ˜… . I do think they are useful however, @sw-yx even pointed out a Formik use case in https://github.com/jaredpalmer/tsdx/issues/365#issuecomment-562632749 and something like #321 could be replaced with a --entry src/**/*.ts glob (which is supported already).

agilgur5 avatar Dec 14 '19 12:12 agilgur5

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 πŸ˜•

agilgur5 avatar Dec 18 '19 01:12 agilgur5

Sorry did not mean to close

jaredpalmer avatar Dec 19 '19 15:12 jaredpalmer

I'm now wondering if just re-running tsdx build and adding --outFile is also just a good thing to have anyways. Where --outFile is the location of the cjs entry (defaults to ./dist/index.js`).

jaredpalmer avatar Dec 31 '19 18:12 jaredpalmer

@jaredpalmer it might be good to have anyway, but for multi-entry specifically, I listed a few of the issues / cons with re-running tsdx build in my opening "Alternatives Considered" section:

Literally 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 file, and still then there'd be a bug that the CJS "entry file" index.js would be overwritten for any entries in the same directory because the name index.js is hard-coded in (until this PR).

--outFile would resolve the last issue of hard-coded index.js, but not the others. Would also have to add --noClean to the build options at a minimum (it's currently exclusive to watch)

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)

agilgur5 avatar Dec 31 '19 21:12 agilgur5

@agilgur5 @jaredpalmer will you know about the progress for next week/ weeks? It's very important topic for me right now, but i'm newbie at the moment with rollup configuration.

czaplej avatar Jan 10 '20 15:01 czaplej

@KrzysztofCzaplejewicz it's not up to me, unfortunately there hasn't been any progress on reviewing or merging this in a few weeks despite its highly requested status and the significant work I put into it. Several of my own PRs have now created merge conflicts with this PR since it's been on-hold for long enough (and I've already fixed several merge conflicts as well).

I do have a dist-multi branch on my fork that you could use in the meantime if you'd like:

npm i -D agilgur5/tsdx#dist-multi

It's a bit behind, equivalent to v0.12.0, but otherwise works just as the docs in this PR say and is well-tested.

agilgur5 avatar Jan 21 '20 10:01 agilgur5

Any chance this could get a second look? Multiple outputs is the only thing stopping me from using tsdx vs using rollup directly (and probably poorly πŸ˜…).

Is there's a recommended approach to multiple outputs for the current version?

angeloashmore avatar Feb 05 '20 19:02 angeloashmore

Sorry for absence, let’s pick this up

jaredpalmer avatar Feb 26 '20 13:02 jaredpalmer

Sorry for absence, let’s pick this up

Ok it's been a week since that was said and it still hasn't been reviewed even once...

I took well over an hour to rebase this with master and fix all the conflicts. I already fixed conflicts before so now I've done this for 10+ conflicts because this hasn't been reviewed a single time in 3 months... (and this was already a pretty time-consuming PR to write-up...)


CI now runs on PRs and so now runs here. The tests all pass, but globbing runs out of memory on Windows because of one of https://github.com/terkelg/tiny-glob/issues/45, https://github.com/terkelg/tiny-glob/issues/30, or https://github.com/terkelg/tiny-glob/pull/44 (which has been fixed, but like TSDX #512, never released). When the glob tests are removed like in https://github.com/jaredpalmer/tsdx/runs/489867527, Windows CI passes.


I've also updated my dist-multi branch and merged in #544, so, again, if anyone wants to use my branch, you can do:

npm i -D agilgur5/tsdx#dist-multi

You can see an example at my very tiny library window-resizeto, which has a ponyfill and polyfill entry.

agilgur5 avatar Mar 06 '20 14:03 agilgur5

Had a breakthrough and figured out how to do code-splitting with multi-entry with some minor changes to the code here thanks to some help in rollup/rollup#3422 .

I've updated the PR and my branch to reflect this.

Now this is straightforwardly better than running tsdx build multiple times, as that wouldn't share code between bundles (on top of being tedious, noisy, and not properly supporting CJS entries). It's also significantly faster, like by an order of magnitude, because not only do we not have to re-run TSDX, we also don't need to run Rollup on each entry, which saves a lot of time in the analysis. The memory errors on Windows also seemed to disappear, possibly because of this.


I've updated my opening PR summary that now the only caveat is that UMD isn't really supported by multi-entry. But UMD is the exception anyway; you can't code-split it and Rollup guides suggest a separate config for it too -- this is probably one of those cases where running tsdx build a second time actually makes sense because the configs are quite different. But maybe we can automate concat'ing all entries together into one big UMD bundle. I'm not sure if that's the way most folks would want to do UMD with multi-entry though?


rollup/rollup#3422 also revealed that there's a big perf improvement TSDX can make in general by re-using the analysis for each output format (currently they're not reused). This might require some very breaking changes to tsdx.config.js due to using per-output plugins (though tsdx.config.js has appropriate warnings about breakage) but can probably hack something non-breaking together.

This does have some minor breakage of tsdx.config.js as well, since with Rollup multi-entry we're using input objects, output.dir, and output.entryFileNames instead of a single input string or output.file. So this should be released in a minor with a potential breakage note. This is used a lot less than plugins in tsdx.config.js though, so I don't think it should be too problematic.

agilgur5 avatar Mar 07 '20 11:03 agilgur5

Thanks @Andarist ! I figured out most stuff since then but still good to get your perspective since you know a bit on the topic.

A consumer of the package should not have to pick which flavor of a package they chose (mainly cjs vs esm).

In my window-resizeto library that I linked above and in #365 as an example, I manually created a polyfill/package.json that specifies where to go for each format.

I still believe that generating appropriate "proxy" directories should be done before releasing this.

As I mentioned in #365 and here, I think auto-generating them (vs. manually as I've done) can be added as a separate feature and has a lot more nuance to it. It effectively requires the developer to cede some control of the publish process to TSDX or to have the proxies generated inside their source directories. Both of which are pretty big things to cede and can have major consequences when not done perfectly.

Additionally, node's conditional exports could be treated as a configuration for TSDX. It seems like a perfect match for this

Yes and no. TSDX needs to know which source files to compile whereas these are dist files. But yea it should match up with what's generated, but because there's no source mapping there's no way to know if the names are right. We could assume equivalent src/dist mapping and figure out what extension the file may have (.js, .jsx, .ts, .tsx) but that's pretty convoluted. Separately TSDX should really warn if main, module, types, and conditional exports don't exist after build (it doesn't do that right now either, so that's a completely new feature). That's about all TSDX can do without a mapping.

sources like in microbundle can be used to configure sources. Or possibly conditional exports with a source field could be optimal?

But configuring sources or source wouldn't handle globs either, while this currently does. πŸ˜•

with the backward compatibility in mind

Idk how to handle the backward compat without resorting to proxy directories, which have the caveat I mentioned above.

agilgur5 avatar Mar 09 '20 06:03 agilgur5

@Andarist any thoughts on the caveats I mentioned above?

Node's conditional exports have had a few bugs / changing definitions in v13.x as well, so I'm wary of recommending those before they're stable too.

Things left to do here:

  • Automatic "proxy directories" for simplicity or documenting how to make them manually
  • Possibly update docs to mention code-splitting and how UMD differs
  • Need to see if backward-compat or an easy migration for tsdx.config.js users who use output.file is possible, since code-splitting chunks requires using output.dir

agilgur5 avatar Mar 22 '20 22:03 agilgur5

Was looking at this library and seeing if it had multi-entry/submodule support and looks like it doesn’t yet. This PR has been open for a long time, is there any hope in getting it reviewed and merged?

jensbodal avatar Jul 18 '20 07:07 jensbodal

Was looking at this library and seeing if it had multi-entry/submodule support and looks like it doesn’t yet. This PR has been open for a long time, is there any hope in getting it reviewed and merged?

I've been using @agilgur5 's fork https://github.com/agilgur5/tsdx.git#dist-multi with great success. Would love to see it land in master πŸ™

kantorcodes avatar Sep 21 '20 21:09 kantorcodes

@kantorcodes anything special you need to do to get it working ? I cant seem to make it work properly.

Morphexe avatar Sep 24 '20 20:09 Morphexe

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!

gustavopch avatar Oct 04 '20 18:10 gustavopch

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?

MMT-LD avatar Oct 13 '20 16:10 MMT-LD

Is there any way to help move this forward? The list of exports of @reduxjs/toolkit is really growing and it would just be great to keep some future apis in their separate entry points.

phryneas avatar Oct 27 '20 07:10 phryneas

@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

  1. the most important leftover is the publish step of auto-generating proxy directories that is fairly out-of-scope and quite intrusive for a build tool, but is also optimal DX. It is left up to the user in the current state of this PR.

    • An optimal solution would be using conditional exports but those are not backward compatible and do not support globs.
      • Conditional exports were also pretty unstable in Node 13.x and caused widespread issues (c.f. #630 etc). There are still open questions about standards for fields too, including for browser ESM / ESM dev/prod and Webpack 5+ (c.f. #631 )
  2. UMD is also unsupported as code-splitting is not possible with UMD. Notably, RTK supports UMD.

  3. the current PR is also breaking to tsdx.config.js users (which also includes RTK). The breaking changes are unavoidable as changes need to be made to the Rollup config to support multi-entry, code splitting, and preserveModules. I had wanted to make some backward compat hacks, but those would likely be more confusing than useful so I had decided against that.

Noteworthy History

This 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. I became the only core contributor as the library fell into disrepair (c.f. #512) and then only maintainer, which is still the case. Back in April I scheduled this breaking change for v0.15.0, which now goes along more neatly with several Rollup changes as well. I'd be remiss if I didn't also mention ongoing world events as well as personal events that fall into this timeline as well. (and, in other issues, people being abusive and attacking me for volunteering hundreds of hours that is incredibly hurtful and damaging and does the opposite of motivating any maintainer)

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).

agilgur5 avatar Oct 27 '20 09:10 agilgur5

@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:

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.

Problems

1. the most important leftover is the publish step of auto-generating proxy directories that is fairly out-of-scope and quite intrusive for a `build` tool, but is also optimal DX. It is left up to the user in the current state of this PR.

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 index.js in every empty folder between the child entry point and the root entry point, importing all children?

   * An optimal solution would be using conditional exports but those are not backward compatible and do not support globs.
     
     * Conditional exports were also pretty unstable in Node 13.x and caused widespread issues (c.f. #630 etc). There are still open questions about standards for fields too, including for browser ESM / ESM dev/prod and Webpack 5+ (c.f. #631 )

2. UMD is also unsupported as code-splitting is not possible with UMD. Notably, RTK supports UMD.

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.

3. the current PR is also breaking to `tsdx.config.js` users (which also includes RTK). The breaking changes are unavoidable as changes need to be made to the Rollup config to support multi-entry, code splitting, and `preserveModules`. I had wanted to make some backward compat hacks, but those would likely be more confusing than useful so I had decided against that.

Yup, you're right about that.

Noteworthy History

This 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. I became the only core contributor as the library fell into disrepair (c.f. #512) and then only maintainer, which is still the case. Back in April I scheduled this breaking change for v0.15.0, which now goes along more neatly with several Rollup changes as well. I'd be remiss if I didn't also mention ongoing world events as well as personal events that fall into this timeline as well.

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 know these PRs just too well - and also how important a real life is. ^^ Which is why I asked how to help best with this, not when it will be released ;)

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 :)

phryneas avatar Oct 28 '20 08:10 phryneas

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 index.js in every empty folder between the child entry point and the root entry point, importing all children?

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: https://unpkg.com/@emotion/[email protected]/base/package.json

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.

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).

Andarist avatar Oct 28 '20 22:10 Andarist

Not quite - it's a directory that lets different tools to pick up the best format of the output bundle.

Yes ^that. I've linked a much smaller example a few times in my previous comments, my own window-resizeto package. It's ~10 LoC, so it should be pretty easy/simple to wrap your head around how everything, including the multi-entry and proxy directory, works.

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. [sic]

@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.

So I'll probably try to experiment with some manually written rollup config.

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. What I've been commenting on is the difficulty to do this with good DX and that the use-cases multi-entry supports don't entirely overlap with what single-entry supports; UMD being the main one I've found. That's not to say you couldn't necessarily use tsdx.config.js to add some UMD feature you want, but I'm also not sure how possible that is given that it'd be separate builds per above (vs. multi-entry is actually all a single, very efficient Rollup build if done correctly -- that's how the code-splitting works as I've mentioned in previous comments).

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).

@Andarist I've seen you mention preconstruct before, but the feature set is not quite the same as TSDX, particularly when it comes to TS. I also see preconstruct actually requires users to add proxy directories manually... which is the same as what this PR currently requires... Your recommendation comes off a bit untoward given that one of the reasons for the stall in this PR is that you had reviewed it and said:

but I still believe that generating appropriate "proxy" directories should be done before releasing this. [...] Additionally, node's conditional exports could be treated as a configuration for TSDX.

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. I've also been looking to simplify the proxy directories here a bit by creating package.json files inside of dist so that, e.g. polyfill/package.json can just include main: '../dist/polyfill/' and not need to think about getting the filenames, extensions, and output formats correct. Putting files inside of dist is also totally within scope and not intrusive. But, even if a good bit simpler, it still requires manual intervention to create polyfill/package.json and that could be easy to get wrong. And the proxy directories sitting in your file tree and package.json#files can get pretty ugly if you have a lot of entrypoints -- as do component libraries.

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... πŸ˜•

agilgur5 avatar Oct 29 '20 16:10 agilgur5

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.

flybayer avatar Oct 29 '20 17:10 flybayer

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 /dist/primitives/index.js file was trying to load ./primitives/index.cjs.development.js instead of ./index.cjs.development.js. The patch I'm using (which I based off @gustavopch's, thanks @gustavopch!) needed to be adjusted on line 207 to strip out everything but the last bit of the path, which I did like this:

const baseLine = `module.exports = require('./${file.includes('/') ? file.split('/')[file.split('/').length - 1] : file}`;

Like I said, there might be a better way to do that, but that's the solution that worked for me.

jnielson94 avatar Oct 30 '20 22:10 jnielson94

@Andarist I've seen you mention preconstruct before, but the feature set is not quite the same as TSDX, particularly when it comes to TS.

I haven't really found anything lacking from preconstruct when it comes to TS support (I use it in several TS repositories)

I also see preconstruct actually requires users to add proxy directories manually... which is the same as what this PR currently requires...

Not quite, you only have to add an extry point in the config (like here) and preconstruct asks you:

A package.json file does not exist for this entrypoint, would you like to create one automatically? (Y/n)

And the proxy directories sitting in your file tree and package.json#files can get pretty ugly if you have a lot of entrypoints -- as do component libraries.

This can be validated (at least to some extent) - if you have the knowledge about what files are generated and you can grab #files then you can totally compare those. You can also pack the library and validate it from there (which is what preconstruct, if you are interested in the implementation - here it is: https://github.com/preconstruct/preconstruct/blob/47515cd533d341c1138f108abc660061eb3486a3/packages/cli/src/validate-included-files.ts

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 :)

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) πŸ˜•

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 "module" condition). From the perspective of a tool like microbundle they should just be validated against the tool-specific config (and ideally auto-fixed as well).

Andarist avatar Oct 30 '20 23:10 Andarist