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

Opting out of typescript checks in Next 9.0+ #7687

Closed
kachkaev opened this issue Jun 27, 2019 · 17 comments
Closed

Opting out of typescript checks in Next 9.0+ #7687

kachkaev opened this issue Jun 27, 2019 · 17 comments

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Jun 27, 2019

Feature request

Is your feature request related to a problem? Please describe.

In Next.js <= 8.1.1 we've been using @zeit/next-typescript, which essentially meant passing ts and tsx files through @babel/preset-typescript. Because of that, imperfections in typings did not prevent the dev server from refreshing or the production app from building. That felt like a convenient DX, given that proper ts checks could be done via yarn lint:tsc in CI (this script would call tsc --noEmit).

I’ve been playing with the latest canary (<= 8.8.1-canary.61) in a couple of new projects and noticed a change in the DX, which I found somewhat distracting. Because TypeScript compiler is now continuously used during development, even a slightest type-related imperfection crashes the app.

The problem with this approach is that it is much harder to experiment with the new stuff. E.g. I can't create a new component, then call it somewhere as <NewComponent foo={data} bar={42} /> and only then implement the typings for the props and handle them. Redundant props are detected by tsc as soon as I mention them, which prevents the app from refreshing. That locks the development process into a certain sequences of steps, which don't always feel natural.

Given that I can run yarn lint:tsc separately, I don't even mind being able to next build in spite of type-related imperfections, as long as the produced JavaScript is sane. This can be useful, for example, when deploying a preview app for a PR that has not been finished yet (the new functionality works, but typings still need a bit of polishing).

Describe the solution you'd like

I understand the motivation behind the new way of tracking TS errors. If I remember correctly, create-react-app does the same. Crashing at each TypeScript imperfection can indeed be useful. For example, it won't let newcomers do the wrong thing and won't overload them with the notion of different kinds of errors.

However, some developers like myself will probably like to avoid tsc-related crashes both during next dev and next build and just rely on yarn lint:tsc as before. Not only this will make experimentation less painful, but also ease project migration into Next.js. We have a few non-Next.js React projects containing a mix of ts and js files. Renaming *.js to *.ts creates quite a few tsc errors, which can't be fixed in one go. The only option is to ensure each new PR does not increase the number of tsc errors until zero is reached.

How feasible would it be to implement a flag in next.config.js? Something like:

module.exports = {
    skipTypescriptChecking: true
    // or
    typescript: {
       transpileOnly: true // same as ts-node --transpile-only
    }
}

Seems like with the flag like that there’s even no need to create tsconfig.json. Next app can sit in a yarn workspace inside a monorepo, where yarn lint:tsc uses a manually crafted top-level tsconfig.json.

Describe alternatives you've considered

Using a stable Next.js release (8.1.1) works well, but it won't be the latest framework version forever.

Additional context

@dmitweb
Copy link

dmitweb commented Jun 27, 2019

@kachkaev it may helps u, i use this in canary version my project from the first version next with ts

    config.plugins = config.plugins.filter(plugin => {
      if (plugin.constructor.name === 'ForkTsCheckerWebpackPlugin') return false;
      return true;
    });

@Timer
Copy link
Member

Timer commented Jun 27, 2019

@kachkaev Hi!

We just merged a pull request that will let the browser load the new JavaScript code before type-checking has been completed. This change will help speed up the feedback loop. You can try this on the latest canary.

I'm not sure if we want to give users the easy control to opt-out of type-checking, as it sounds like this use case may be better suited by turning off strict: true and running your lint step with a more strict configuration.

We'd rather release this as-is, and if there's large community request/support we might run through different iterations (e.g. dismissable typescript error so you can interact with the app, or just a persistent toast in the bottom right of the screen) before we just give any easy way to "turn it off".

For now, totally turning it off might be better accomplished by the above suggestion. ☝️

@Timer Timer closed this as completed Jun 27, 2019
@ericjuta
Copy link

ty for this @dmitweb

Just spent 6 hours trying to upgrade next.js today
apollo codegen adds ts-ignore to its generates gql typings so the typechecker wasn't finding the modules upon build

@kachkaev
Copy link
Contributor Author

kachkaev commented Jun 28, 2019

Thanks for your reply @Timer. A workaround by @dmitweb is awesome – it removes most of the pain.

It would be great if you could reconsider this issue if it's marked with a few more 👍. In addition to what's possible already, transpileOnly could be useful for skipping tsconfig.json creation. This file is redundant if a Next.js app is inside a monorepo package and all TypeScript in all packages is linted with a monorepo-wide yarn lint:tsc.

@kachkaev
Copy link
Contributor Author

kachkaev commented Jun 28, 2019

One more small thought: This line still periodically shows up in the terminal despite that ForkTsCheckerWebpackPlugin is removed as a workaround:

[ info ]  bundled successfully, waiting for typecheck results ...

It's clear why so, I'm just flagging this as a second reason for introducing transpileOnly option (the first one is to do with redundancy of tsconfig.json).

Could there also be a performance lag even when ForkTsCheckerWebpackPlugin is removed from the list of plugins?

@dmitweb
Copy link

dmitweb commented Jun 28, 2019

@kachkaev maybe this because in canary v61 async check, i'm still at v60, didn't see message like this.

@kachkaev
Copy link
Contributor Author

@kachkaev kachkaev changed the title Opting out of typescript checks on canary Opting out of typescript checks in Next 9.0+ Jul 9, 2019
@chrisblossom
Copy link

@Timer Please reopen this. This "feature" is not part of a good development workflow and actually gets in the way of development.

I'm not sure if we want to give users the easy control to opt-out of type-checking, as it sounds like this use case may be better suited by turning off strict: true and running your lint step with a more strict configuration.

You should not be recommending people set strict: false as a way to "mitigate" next.js from giving compile errors in the browser. Instead, next.js should provide a UX workflow that works with existing IDEs and doesn't get in the way of development. If someone's editior doesn't support inline typescript errors (which editior doesn't?), why would they be using typescript in next.js?

@paintedbicycle
Copy link

paintedbicycle commented Jul 18, 2019

Yeah this is a big issue for me, in upgrading to nextjs9. I was running typescript compiler before (nextjs8), and am slowly migrating a codebase that wasn't written in typescript over to it. But now I can't use nextjs9 as my project won't build at all.

I get that typescript is here to help you, and even that the default setting could be to crash out on TS error, but I think it makes sense for me to be able to override that until I'm ready. It worked fine before with the next-typescript plugin,

In fact, it appears I cannot even go back to the old typescript setup using the next-typescript plugin and babel config with nextjs9, so now I can't build my project or have to not update next

@chj-damon
Copy link

I ran into this issue because it was exactly what @kachkaev describled:
[ info ] bundled successfully, waiting for typecheck results ...

I have two pages: index.tsx, about/index.tsx:

index.tsx:
export default () => (
  <div>
    <Link href="/about"><a>got to about page</a></Link>
  </div>
);

about/index.tsx:
export default () => (
  <div>
    this is about page.
  </div>
);

it works fine. But if I add some style in about.tsx:

import styles from './index.module.less';
export default () => <div className={styles.content}>123</div>;

I cannot link to about page anymore. and I got this info in the terminal:

[ wait ]  compiling ...
[ info ]  bundled successfully, waiting for typecheck results ...
[ event ] disposing inactive page(s): /next/dist/pages/_error, /about

My next.config.js is:

const withLess = require('@zeit/next-less');
const path = require('path');

module.exports = withLess({
  typescript: {
    transpileOnly: true,
  },
  cssModules: false,
  cssLoaderOptions: {
    importLoaders: 1,
    localIdentName: '[name]___[local]___[hash:base64:5]',
  },
  lessLoaderOptions: {
    javascriptEnabled: true,
  },
  webpack(config) {
    config.module.rules.push({
      test: /\.js$/,
      enforce: 'pre',
      include: [path.resolve('components'), path.resolve('pages')],
    });
    config.devtool = 'cheap-module-inline-source-map';
    config.resolve.alias = {
      ...config.resolve.alias,
      '@pages': path.join(__dirname, '.', 'pages'),
      '@components': path.join(__dirname, '.', 'components'),
    };
    return config;
  },
});

@jeremy-coleman
Copy link

jeremy-coleman commented Jul 28, 2019

easiest fix. you have to point the tsconfig to a real directory in order for it not to throw on start. it will only check this 1 file this way, and you can continue to live your life
image

@callumlocke
Copy link
Contributor

@Timer please reconsider your position on this.

I'm not sure if we want to give users the easy control to opt-out of type-checking

Just to reassure you, no one here is trying to to "opt out of type-checking", otherwise we'd just switch to plain JavaScript! We just want to opt out of type-checking during the Next build.

I think it's great that you offer a simple way to fail the Next build on type errors, for the many users who still prefer that behaviour. And I don't even mind that it's the default. But it is frustrating that you don't trust our judgement when we want to opt out of it. Our preferred workflow is no less rigorous or valid. We just enforce type-checking at a different point in the lifecycle, along with our unit tests and linters. Some teams enforce it as a pre-commit hook, others enforce it as a PR merge condition or during a CI deployment job.

@fabb
Copy link
Contributor

fabb commented Aug 2, 2019

There is another usecase for making the options accessible: opting IN to tslint/eslint checking during Fork TS Checker runs.

We had that on happily in previous next versions, but since next 9 we run into problems: #7936 (comment)

@Timer
Copy link
Member

Timer commented Aug 11, 2019

We'll accept a PR adding this! Please see #8331.

@threehams
Copy link

In the meantime, here's a newer workaround which still runs the checker (prevents the server from hanging) but allows anything:

next.config.js:

const ForkTsCheckerWebpackPlugin = require("fork-ts-checker-webpack-plugin");

module.exports = phase => {
  return {
    webpack(config) {
      // remove existing plugin
      config.plugins = config.plugins.filter(plugin => {
        return plugin.constructor.name !== "ForkTsCheckerWebpackPlugin";
      });
      // only report errors on a matcher that doesn't match anything
      config.plugins.push(
        new ForkTsCheckerWebpackPlugin({
          reportFiles: ["does-not-exist"],
        }),
      );
      return config;
    },
  };
};

@kachkaev
Copy link
Contributor Author

This should be fixed by #9138 🤞

@balazsorban44
Copy link
Member

This issue has been automatically locked due to no recent activity. If you are running into a similar issue, please create a new issue with the steps to reproduce. Thank you.

@vercel vercel locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests