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

fix/deps: upgrade rollup-plugin-typescript2 to fix cache issue #896

Merged
merged 2 commits into from
Oct 12, 2020

Conversation

tanem
Copy link
Contributor

@tanem tanem commented Oct 6, 2020

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?

@vercel

This comment has been minimized.

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 6, 2020

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: ezolenko/rollup-plugin-typescript2#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 ezolenko/rollup-plugin-typescript2#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.

agilgur5
agilgur5 previously approved these changes Oct 6, 2020
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, though I'd like to understand root cause and why workarounds didn't work per my above comment

@tanem
Copy link
Contributor Author

tanem commented Oct 6, 2020

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.

@agilgur5 agilgur5 changed the title fix: update rollup-plugin-typescript2 to fix build issue fix: update rollup-plugin-typescript2 to fix cache issue Oct 6, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 7, 2020

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.
#691 made it so it was a single cache instead of a per-format cache (because the source doesn't change and the config is hashed anyway) so I guess it may have created a race??

Will have to follow up on this upstream

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 7, 2020

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 #889 (comment) . 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

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 7, 2020

I had checked this already when looking to update previously, but confirming here that I see no relevant breaking changes in rpts2 v0.27.0. The release notes aren't particularly detailed but the diff was small enough to read through and check.

@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 12, 2020

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 ezolenko/rollup-plugin-typescript2#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.. 🤔

tanem and others added 2 commits October 12, 2020 17:36
- fixes a cache issue with certain builds by upgrading to the most
  recent version of rollup-plugin-typescript2, v0.27.3, which just had
  a PR and release for this specific cache issue 2 weeks ago
  - there were no relevant breaking changes in the upgrade from rpts2
    v0.26 to v0.27.0, so this should be a straight bugfix
- there was never a test that built all, meaning tests left out the
  somewhat popular UMD build in particular
  - per my investigation, CJS + UMD, CJS + System, UMD + System started
    bugging out recently due to an upstream bug, but this wasn't known
    proactively because there were no tests for it
    - and I also found that CJS + System actually was bugging out in
      the previous version, TSDX v0.13.3, but I'm guessing no one
      reported it back then as it's an unpopular combination of formats

- this test fails prior to upgrade of rpts2 to v0.27.3 and succeeds
  after the upgrade
  - so this should act as a regression test against this bug as well

- created a new e2e build-options test file for this because
  build-default is meant to have 0 options and test only the defaults
  - had to give it a differentiated "stage" name as it uses the same
    build-default fixture and so breaks during test parallelization
    without that
  - should also move the regeneratorRuntime `--target node` test here
    since that's an option and not a default
@agilgur5 agilgur5 changed the title fix: update rollup-plugin-typescript2 to fix cache issue fix/deps: upgrade rollup-plugin-typescript2 to fix cache issue Oct 12, 2020
@agilgur5 agilgur5 added the kind: bug Something isn't working label Oct 12, 2020
Copy link
Collaborator

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test passes, no weird logs in the output (aside from #884 (comment) which is an existing issue). Test conforms to existing test style and pretty straightforward so that LGTM to me as well.

@agilgur5 agilgur5 merged commit e2f1b76 into jaredpalmer:master Oct 12, 2020
@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 12, 2020

@allcontributors please add @tanem for code, bug (the create repro and workaround not working detail made this an actionable bug report as well).

@allcontributors
Copy link
Contributor

@agilgur5

I've put up a pull request to add @tanem! 🎉

@JustFly1984

This comment has been minimized.

@agilgur5

This comment has been minimized.

@JustFly1984

This comment has been minimized.

@tanem
Copy link
Contributor Author

tanem commented Oct 14, 2020

Thanks for the all the work here and the merge @agilgur5 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug Something isn't working scope: dependencies Pull requests that update a dependency file topic: rollup-plugin-typescript2 Issues and PRs relating to rpts2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v0.14.0 rpts2 cache issue when building umd with cjs v0.14.0 rpts2 cache issue with Yarn workspaces
3 participants