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

v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done()) #421

Closed
jschill opened this issue Sep 14, 2022 · 6 comments · Fixed by #422
Closed

v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done()) #421

jschill opened this issue Sep 14, 2022 · 6 comments · Fixed by #422
Labels
kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc kind: regression Specific type of bug -- past behavior that worked is now broken problem: no repro No reproduction was provided (and have not tried to repro without one) scope: cache Related to the cache scope: tests Tests could be improved. Or changes that only affect tests

Comments

@jschill
Copy link

jschill commented Sep 14, 2022

What happens and why it is incorrect

On v0.34 I get:
TypeError: Cannot read property 'done' of undefined
...and it points to cache.done() in index.ts that was recently changed.

Downgrading to v0.33.0.1 v0.32 "solves" the problem.

Environment

Versions

System:
    OS: macOS 12.5.1
    CPU: (4) x64 Intel(R) Core(TM) i7-7567U CPU @ 3.50GHz
    Memory: 103.09 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 14.20.0 - ~/.nvm/versions/node/v14.20.0/bin/node
    Yarn: 1.22.18 - ~/.yarn/bin/yarn
    npm: 6.14.17 - ~/.nvm/versions/node/v14.20.0/bin/npm
    Watchman: 2022.08.29.00 - /usr/local/bin/watchman
  npmPackages:
    rollup: 2.79.0 => 2.79.0
    rollup-plugin-typescript2: ^0.34.0 => 0.34.0

rollup.config.js

:
import ts from 'rollup-plugin-typescript2';
export default {
  input: 'src/client/index.ts',
  plugins: [
    ts({
      tsconfig: './tsconfig.json',
      check: false,
      verbosity: 3
    })],
  output: {
    dir: 'output',
    format: 'cjs'
  },
};

tsconfig.json

:
{
    "extends": "../tsconfig.base.json",
    "transpileOnly": true,
    "compilerOptions": {
        "rootDir": "./..",
        "baseUrl": "./src",
        "allowJs": true,
        "target": "es3",
        "jsx": "react",
        "jsxFactory": "h",
        "jsxFragmentFactory": "Fragment",
        "noLib": false,
        "outDir": "build/",
        "noEmitHelpers": true,
        "preserveConstEnums": true,
        "suppressImplicitAnyIndexErrors": true,
        "importHelpers": true,
        "noUnusedLocals": true,
        "noUnusedParameters": true,
        "sourceMap": true,
        "inlineSources": true,
        "allowSyntheticDefaultImports": true
    },
    "include": [
        "src/**/*.js",
        "src/**/*.jsx",
        "src/**/*.ts",
        "src/**/*.tsx"
    ],
    "exclude": [
        "node_modules",
        "**/*.spec.ts"
    ]
}

package.json

:

plugin output with verbosity 3

:
npm run rollup:build

> @ rollup:build /Users/johannes/Documents/workspace/sirjunkalot/packages/web
> rollup --config rollup.config.js


src/client/index.ts → output...
[!] (plugin rpt2) TypeError: Cannot read property 'done' of undefined
TypeError: Cannot read property 'done' of undefined
    at buildDone (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup-plugin-typescript2/src/index.ts:82:9)
    at Object.buildEnd (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup-plugin-typescript2/src/index.ts:295:5)
    at /Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/shared/rollup.js:22848:40
    at async Promise.all (index 0)
    at PluginDriver.hookParallel (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/shared/rollup.js:22776:9)
    at /Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/shared/rollup.js:23744:13
    at catchUnfinishedHookActions (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/shared/rollup.js:23216:20)
    at rollupInternal (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/shared/rollup.js:23734:5)
    at build (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/bin/rollup:1528:20)
    at runRollup (/Users/johannes/Documents/workspace/sirjunkalot/packages/web/node_modules/rollup/dist/bin/rollup:1668:21)
@agilgur5 agilgur5 changed the title v0.34 - TypeError: Cannot read property 'done' of undefined - [cache.done()] v0.34 - TypeError: Cannot read property 'done' of undefined (cache.done()) Sep 14, 2022
@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 14, 2022

Thanks for the report @jschill .

Per the issue template, could you provide a reproduction of this error?

All tests pass (and I've used 0.34.0 in a few projects already), so this sounds like it must be an edge case that isn't covered by the test suites (yet).

The only way for cache to be undefined is if you hit some error during the buildStart hook, which normally never has errors as it's just an initializer.
Your logs point to this line as well, which is an error case conditional.

This would be a simple fix, but it would be good to have a repro as this is an unexpected edge case, meaning we potentially have a bug or something in buildStart too

Downgrading to v0.33.0.1 v0.32 "solves" the problem.

Also is there a reason why 0.33.0 didn't work for you? Since this change is only in 0.34.0

@agilgur5 agilgur5 added problem: needs more info This issue needs more information in order to handle it kind: regression Specific type of bug -- past behavior that worked is now broken problem: no repro No reproduction was provided (and have not tried to repro without one) labels Sep 14, 2022
@jschill
Copy link
Author

jschill commented Sep 14, 2022

Hello @agilgur5

Per the issue template, could you provide a reproduction of this error?

My project is quite large so there's no easy way for me to create a reproduction. The tl;dr is that I'm trying to change bundler due to Im tired of 1.30m builds, so I'm evaluating different ones and when I came to rollup with this plugin it increased my build time to 2.30m so I kind of moved on pretty quickly :-) If I get some time over I can try to revert and reproduce it.

Downgrading to v0.33.0.1 v0.32 "solves" the problem.

Also is there a reason why 0.33.0 didn't work for you? Since this change is only in 0.34.0

First I tried 0.34.0 - did not work.
Downgraded to 0.31.0 I think and then upgraded version by version and everything worked* until 0.34.0, including 0.33.0.1.
Then downgraded to 0.33.0.1 again and for some reason it did not work this time. Maybe cache issue or a classic node you-need-to-reinstall-all-node_modules. Continued the downgrade to 0.32.0 and then it started to work again so that's why I changed my error report.

*Oh! It should probably also be mentioned that WHEN it "worked" again it didn't actually work, because I got another error saying I needed to change "module" to "ES2015" or "ESNext" because rollup doesn't support "commonjs". Maybe that is the edge case we're looking for.

@agilgur5
Copy link
Collaborator

agilgur5 commented Sep 14, 2022

The tl;dr is that I'm trying to change bundler due to Im tired of 1.30m builds, so I'm evaluating different ones and when I came to rollup with this plugin it increased my build time to 2.30m so I kind of moved on pretty quickly :-)

Gotcha. I'd recommend trying Vite or Parcel if performance is what you're looking for. And, more generically, probably use esbuild or swc instead of a plugin based on the TypeScript Compiler, which will most certainly be slower.

rpt2, in particular, prefers correctness over performance by default, as you probably noticed from setting check: false (other optimizations include tsconfig declaration: false, skipLibCheck: true, etc -- can check the "optimization" label here).

*Oh! It should probably also be mentioned that WHEN it "worked" again it didn't actually work, because I got another error saying I needed to change "module" to "ES2015" or "ESNext" because rollup doesn't support "commonjs". Maybe that is the edge case we're looking for.

Ah yep, that's the edge case indeed! Thanks for that bit of info, very helpful!

That comes from this line, which is indeed called during initialization in the buildStart hook, so that adds up!

And that line also got changed in another commit in this recent release from a simple throw statement to a Rollup context.error (previously this wasn't possible), which will then trigger the buildEnd error edge case... and hit this bug 🐞

I actually wrote unit tests for this specific tsconfig error, but not an integration test that would capture the interaction b/t these two hooks.

Appreciate the helpful passing-by bug report @jschill 🙂

@agilgur5 agilgur5 removed the problem: needs more info This issue needs more information in order to handle it label Sep 14, 2022
@agilgur5 agilgur5 changed the title v0.34 - TypeError: Cannot read property 'done' of undefined (cache.done()) v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done()) Sep 14, 2022
@agilgur5 agilgur5 changed the title v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done()) v0.34.0 - TypeError: Cannot read property 'done' of undefined (cache.done()) Sep 14, 2022
@agilgur5 agilgur5 added priority: in progress scope: cache Related to the cache scope: tests Tests could be improved. Or changes that only affect tests labels Sep 14, 2022
@jschill
Copy link
Author

jschill commented Sep 15, 2022

Gotcha. I'd recommend trying Vite or Parcel if performance is what you're looking for. And, more generically, probably use esbuild or swc instead of a plugin based on the TypeScript Compiler, which will most certainly be slower.

:-) That was my plan as well and that's actually where I'm coming from. Parcel gave me lots of problems since i have a "poor mans monorepo"-setup with nested node_modules. The vite-integration worked but I wasn't happy with the integration to get SSR working. Plus since they don't bundle in dev-mode and deliver es modules - and I haven't implemented code splitting - on page load there were 690+ requests done so time to render was roughly 20 seconds. That DX is horrible, so I need to implement code splitting and that is (hopefully) out-of-scope for what I'm trying to achieve. Might go back and try one of those tools if I don't find another solution.

Appreciate the helpful passing-by bug report @jschill 🙂

👍

@agilgur5
Copy link
Collaborator

Added a 1 char fix for this in #422, as well as a new integration test for this edge case 🙂

@agilgur5 agilgur5 added the kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc label Sep 17, 2022
@agilgur5
Copy link
Collaborator

agilgur5 commented Oct 3, 2022

Released in 0.34.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: dx Improvements to dev experience, e.g. error messages, logging, external-facing docs, etc kind: regression Specific type of bug -- past behavior that worked is now broken problem: no repro No reproduction was provided (and have not tried to repro without one) scope: cache Related to the cache scope: tests Tests could be improved. Or changes that only affect tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants