Closed tanem closed 3 years ago
This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.
🔍 Inspect: https://vercel.com/formium/tsdx/hlgmed9ue
✅ Preview: https://tsdx-git-fix-rpts2-cache-issue.formium.vercel.app
Thanks for the patch @tanem -- was actually looking to bump this myself once rpts2 v0.28.0 is released, because that will contain my fix that was recently merged: https://github.com/ezolenko/rollup-plugin-typescript2/pull/221 (which resolves a few issues and does a root cause fix for a PR of mine in TSDX). Though that requires a few extra changes and some testing, so maybe I'll do that as a separate PR then. But was planning to fix both / upgrade all the way up for TSDX v0.14.1.
and the workaround mentioned here didn't work for me.
If that didn't work for you could you provide more details on that? Neither nuking the cache nor all of node_modules
worked? Jared had confirmed the latter too.
That might suggest https://github.com/ezolenko/rollup-plugin-typescript2/pull/243 isn't a root cause fix then. Which may mean there's some broken caching 😕
Though now I think I understand why it's occurring, it's because the cache dir was changed/set to the default in #691 . But if everything is nuked that should be the same as a new install / fresh slate, which should be handled... And #691 was released in TSDX v0.13.3... And cache only gets rolled after a build, not before... 🤔 🤔 🤔
Tested by yarn linking tsdx across repos, and the build looks to be working ok with this change.
💯
Guess dependabot would have eventually updated this dep
I've temporarily disabled it because all the dep updates were spammy (duplicate PRs, unnecessarily updating patches/minors, updating peerDeps that can't be updated in isolation, etc). Need to add a bunch of filters to it, but even then, it's not feasible, not always necessary, and may actually be a bad idea stability-wise to update deps immediately after release. Whereas a PR like this or bugs reported actually demonstrate a need to update.
Hey @agilgur5 👋
Here's what I did to get the issue:
$ npx tsdx create my-lib
[chose react template]
$ cd my-lib
$ yarn build --format cjs,esm,umd
[error]
Then I tried:
$ rm -rf node_modules/.cache/rollup-plugin-typescript2/
$ yarn build --format cjs,esm,umd
[error]
Finally:
$ rm -rf node_modules
$ yarn
$ yarn build --format cjs,esm,umd
[error]
Once I applied the update in this PR to my local copy of tsdx
, then linked that over to my-lib
, yarn build --format cjs,esm,umd
ran without error.
On the topic of Dependabot, have you considered using Renovate instead? I find it gives a lot more control in terms of how much noise you have to deal with as a maintainer. It also has a stabilityDays
config option which addresses your point about updating immediately following a release.
One thing it doesn't do that Dependabot does do is update transient dependencies for security updates (hopefully I described that correctly, don't want to misrepresent Renovate 😅). In those cases, I've seen other open-source repos use Dependabot just for security patches, and Renovate for the rest. For my own repos I usually manually update if I need a security patch applied promptly, otherwise I let the usual Renovate update cycle handle it.
Thanks for the additional details @tanem
$ npx tsdx create my-lib [chose react template] $ cd my-lib $ yarn build --format cjs,esm,umd [error]
This is pretty intriguing; I thought most folks, like Jared, were getting this during an upgrade of TSDX, but this is happening to you on a fresh install.... yea that's even more confusing since rpts2 hasn't been updated since TSDX v0.13.0 / #506 . And #691 / #758 were the only changes to the options and were in v0.13.3... but for some reason this wasn't discovered until v0.14.0 🤔 🤔
The cache code initiates both directories in its constructor which would by definition be called before its roll()
call... weird.
Will have to follow up on this upstream
On the topic of Dependabot, have you considered using Renovate instead?
I have not. I didn't realize Renovate did dependencies, I thought it was just an automated merge bot based on past usage I'd seen.
It seems to have an enormous list of configuration options that are potentially useful, though even those don't handle cases like https://github.com/formium/tsdx/pull/889#discussion_r496208822 . From a somewhat quick look, I'm also not sure if most of the options outside of stabilityDays
would be something I'd use or would add more than Dependabot anyway.
Dependencies are pretty manual in my experience; updating all the time without a defined need just adds a lot of noise.
Also recently switched from Greenkeeper to have even more issues with Snyk (as linked above), then to Dependabot (#846 ), so I'm not clamoring to change and learn another potentially problematic bot, especially if I need to use two for security issues (which are the few that are quite important / aren't noise) 😕 But I'll keep it in my mind for the future, thanks
Reproduced and confirmed #892 as well as did some more investigations there, including finding that this bug did start with TSDX v0.13.3, just with less popular combinations of formats than v0.14.0 for unknown reason.
I'm not going to wait on the release of https://github.com/ezolenko/rollup-plugin-typescript2/pull/221 / rtps2 v0.28.0 anymore since this issue seems to be quite widespread, #898 and #900 hit into it as well.
I'll probably merge and release TSDX v0.14.1 tomorrow, but I'd like to add a smoke test that tests a build of all formats simultaneously to confirm this works and doesn't regress. Apparently there aren't any tests of umd
or system
, can't remember why I didn't add them when I built out a lot of the test suite in #627... maybe I was just sticking to integration tests then.. 🤔
@allcontributors please add @tanem for code, bug (the create
repro and workaround not working detail made this an actionable bug report as well).
@agilgur5
I've put up a pull request to add @tanem! :tada:
@agilgur5 I have the feedback. I had released 2 new versions of my library @react-google-maps/api
1.12.1 includes tsdx update to 0.14.1 1.13.0 includes updated webpack@5.0.0
I see the bundle size grow from 15.2kb to 17.5kb for no obvious reason. https://bundlephobia.com/result?p=@react-google-maps/api@1.12.1 - bundle size is grown after update tsdx https://bundlephobia.com/result?p=@react-google-maps/api@1.13.0 - bundle size did not changed by updating webpack version
There is definitely some degradation in build tool optimizations.
for no obvious reason. There is definitely some degradation in build tool optimizations.
@JustFly1984 you upgraded from v0.13.3 to v0.14.1, i.e. two versions, one of which was a breaking change. The v0.14.0 Release Notes are very explicit as to possible bundle size increases and even give directions on how to reduce. Please read the release notes before upgrading, especially to breaking releases.
As such, this is unrelated to v0.14.1 or this PR, so this is not the right place either.
@agilgur5 thank you for clarifications!
Thanks for the all the work here and the merge @agilgur5 🙏
Bumped into #888/#892 when creating a new repo, and the workaround mentioned here didn't work for me.
This patch brings in the permanent fix from ezolenko/rollup-plugin-typescript2#243. Tested by yarn linking tsdx across repos, and the build looks to be working ok with this change.
Guess dependabot would have eventually updated this dep, but figured it was important enough to do ASAP?