Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Typescript middleware proposal #1422

Closed
davidje13 opened this issue Jun 23, 2019 · 17 comments
Closed

Typescript middleware proposal #1422

davidje13 opened this issue Jun 23, 2019 · 17 comments
Labels

Comments

@davidje13
Copy link
Contributor

This isn't at the point where it could be a PR, but is more detailed than the discussion in #1269, so I wanted to create a separate discussion for it. If the maintainers disagree with this split please feel free to mark as a duplicate of 1269.

The current state of TypeScript is quite good for integration. Babel compilation is supported, and the recommended linting tool has switched from TSLint to ESLint (already available in Neutrino). Integration with Jest is also straightforward.

This configuration adds typescript support for compilation, testing with Jest, and linting on top of the AirBnB ESLint configuration. It performs type checking as a lint-time task, not a build-time task, in keeping with the recommended approach when integrating with babel.

It is not yet self-contained. I would like to get some feedback on how it could be made more modular, so that it can be made into proper middleware.

.neutrinorc.js

const airbnb = require('@neutrinojs/airbnb');
const jest = require('@neutrinojs/jest');
const node = require('@neutrinojs/node');

const typescript = () => (neutrino) => {
  // This, and the babel preset below, is all that is required for basic compilation
  // Can the babel preset be added here somehow?
  // How does neutrino merge babel config between sibling packages?
  const { extensions } = neutrino.options;
  const index = extensions.indexOf('js');
  extensions.splice(index, 0, 'ts', 'tsx');
  neutrino.options.extensions = extensions;
};

module.exports = {
  use: [
    typescript(), // Order matters; how can this be avoided?

    // Linter config needs to have access to the neutrino object for "import/resolver"
    (neutrino) => neutrino.use(airbnb({
      eslint: {
        // Can these parser options be specified from the typescript middleware?
        parser: '@typescript-eslint/parser',
        parserOptions: {
          project: './tsconfig.json',
        },

        // All typescript-eslint references could plausibly be extracted into separate typescript-eslint middleware
        // How to merge with airbnb config is still a question though
        plugins: ['@typescript-eslint'],
        baseConfig: {
          extends: [
            'plugin:@typescript-eslint/eslint-recommended',
            'plugin:@typescript-eslint/recommended',
          ],
          settings: {
            // Can this resolver rule be set by default in the eslint middleware? It isn't typescript-specific
            'import/resolver': {
              node: {
                extensions: neutrino.options.extensions.map((ext) => `.${ext}`),
              },
            },
          },
        },
      },
    })),

    jest(),

    node({ // Could be one of many middlewares depending on the project
      babel: {
        presets: [
          ['@babel/preset-typescript', {}],
        ],
      },
    }),
  ],
};

New dependencies

  • @babel/preset-typescript
  • typescript (technically not required for pure babel compilation, but not much use in that state)

New dependencies if used with Jest

  • @types/jest

New dependencies if used with ESLint

  • @typescript-eslint/eslint-plugin
  • @typescript-eslint/parser

New files

tsconfig.json

{
  "compilerOptions": {
    "target": "esnext",
    "moduleResolution": "node",
    "allowJs": true,
    "noEmit": true,
    "strict": true,
    "isolatedModules": true,
    "esModuleInterop": true
  },
  "include": [
    "src"
  ]
}

There is no .js version of this config file available; see microsoft/TypeScript#25271

The use of "strict": true is a user choice. The other options are required. The include list should ideally come from neutrino, but this file cannot be a script.

Script updates:

{
  "scripts": {
    "lint": "eslint --cache --format codeframe --ext mjs,jsx,js,tsx,ts src && tsc"
  }
}

(tsc is added as a separate step, and ts,tsx must be added to the --ext flag; see eslint/eslint#2274)

Remaining integrations

  • Prettier is supposedly well supported but I haven't tried to integrate it
  • The configurations above could easily be applied to airbnb-base, react, vue, web, etc. but it would be better if applied to eslint and compile-loader directly in all cases. I don't know how neutrino manages merging configuration between siblings.
  • I don't know how this would integrate with Karma or Mocha
@eliperelman
Copy link
Member

Thank you for this! I think this is a good place to start a conversation. First up, what happens if we do not use the typescript package? Can we remove the tsconfig.json file if so?

@davidje13
Copy link
Contributor Author

davidje13 commented Jun 23, 2019

Without the typescript package, babel is still able to convert ts into js just fine (and doesn't need a tsconfig.json file; it doesn't use it even if present as far as I can tell). That means compilation and testing (Jest) continue to work.

BUT: tsc will (obviously) not work, and that's the only command which actually checks the type safety. So you could have code like function add(a: number, b: number): string { return a + b; } and everything would be happy, but you're not getting the type-checking benefits of typescript. Only tsc will show that there's a return value mismatch. Note that "noEmit": true is what makes it behave like a linter; without that it would actually do a full compilation, bypassing babel entirely. That config comes from the official announcement.

Also for some reason the @typescript-eslint/parser eslint package stops working. It doesn't call out typescript as a peerDependency, but it does need it.

@davidje13
Copy link
Contributor Author

From some of the hints which appeared recently in #1129 from an unrelated conversation, I've managed to get a more modular implementation.

The main remaining problem is the order in which things must be applied; it seems that some parts must be used before other modules, and some parts after, so this isn't very practical yet.

Is there an API for deferring configuration until after the other plugins have been loaded?

const merge = require('deepmerge');

module.exports = {
  typescriptBefore: () => (neutrino) => {
    const { extensions } = neutrino.options;
    const index = extensions.indexOf('js');
    extensions.splice(index, 0, 'ts', 'tsx');
    neutrino.options.extensions = extensions;
  },

  typescriptAfter: () => (neutrino) => {
    neutrino.config.module.rule('compile').use('babel').tap((options) => {
      options.presets.push(['@babel/preset-typescript', {}]);
      options.plugins.push(['@babel/plugin-proposal-class-properties', {}]);
      options.plugins.push(['@babel/plugin-proposal-object-rest-spread', {}]);
      return options;
    });

    const lintRule = neutrino.config.module.rule('lint');
    if (lintRule) {
      lintRule.use('eslint').tap((lintOptions) =>
        lintOptions.useEslintrc ? lintOptions : merge(lintOptions, {
          parser: '@typescript-eslint/parser',
          parserOptions: {
            project: './tsconfig.json',
          },
          plugins: ['@typescript-eslint'],
          baseConfig: {
            extends: [
              'plugin:@typescript-eslint/eslint-recommended',
              'plugin:@typescript-eslint/recommended',
            ],

            // This part could still be part of eslint config itself
            settings: {
              'import/resolver': {
                node: {
                  extensions: neutrino.options.extensions.map((ext) => `.${ext}`),
                },
              },
            },
          },
        })
      );
    }
  },
};

New usage:

module.exports = {
  use: [
    typescriptBefore(),
    airbnb(),
    jest(),
    node(),
    typescriptAfter(),
  ],
};

I also realised that some additional dependencies are needed for babel to generate valid code for current platforms (@babel/plugin-proposal-class-properties and @babel/plugin-proposal-object-rest-spread, although the latter is only really needed for IE/Edge now)

It still needs tsconfig.json, and I don't think it will be possible to avoid that without running the compiler programmatically rather than through the tsc commandline, which goes against the philosophy introduced in v9. It would also remove compatibility with editors, etc.

@davidje13
Copy link
Contributor Author

OK, so I've managed to make this into a single piece of middleware (well, 2 because I decided splitting linting into a separate function would make more sense)

This works, but… it's super hacky. I don't really understand the intent of the abstractions in webpack-chain, and looking through the code of existing modules makes me think every module is currently inventing its own hacks to work around some fundamental limitations here. Happy to be proven wrong though!

const merge = require('deepmerge');

function patchMethod(o, methodName, replacement) {
  const original = o[methodName].bind(o);
  o[methodName] = replacement.bind(o, original);
  return o;
}

function interceptAtEnd(neutrino, interceptRuleName, interceptUseName, fn) {
  let applied = false;

  patchMethod(neutrino.config.module, 'rule', function(originalRule, ruleName) {
    return patchMethod(originalRule(ruleName), 'use', function(originalUse, useName) {
      return patchMethod(originalUse(useName), 'get', function(originalGet, getName) {
        if (ruleName === interceptRuleName && useName === interceptUseName && !applied) {
          applied = true;
          this.tap(fn);
        }
        return originalGet(getName);
      });
    });
  });

  patchMethod(neutrino.config, 'toConfig', function(originalToConfig, ...args) {
    if (!applied) {
      applied = true;
      const rule = neutrino.config.module.rule(interceptRuleName);
      if (rule) {
        rule.use(interceptUseName).tap(fn);
      }
    }
    return originalToConfig(...args);
  });
}

module.exports = {
  typescript: () => (neutrino) => {
    const { extensions } = neutrino.options;
    const index = extensions.indexOf('js');
    extensions.splice(index, 0, 'ts', 'tsx');
    neutrino.options.extensions = extensions;

    interceptAtEnd(neutrino, 'compile', 'babel', (options) => {
      options.presets.push(['@babel/preset-typescript', {}]);
      options.plugins.push(['@babel/plugin-proposal-class-properties', {}]);
      options.plugins.push(['@babel/plugin-proposal-object-rest-spread', {}]);
      return options;
    });
  },

  typescriptLint: () => (neutrino) => {
    interceptAtEnd(neutrino, 'lint', 'eslint', (options) => {
      if (options.useEslintrc) {
        return options;
      }
      return merge(options, {
        parser: '@typescript-eslint/parser',
        parserOptions: {
          project: './tsconfig.json',
        },
        plugins: ['@typescript-eslint'],
        baseConfig: {
          extends: [
            'plugin:@typescript-eslint/eslint-recommended',
            'plugin:@typescript-eslint/recommended',
          ],
          settings: {
            'import/resolver': {
              node: {
                extensions: neutrino.options.extensions.map((ext) => `.${ext}`),
              },
            },
          },
        },
      })
    });
  },
};

What's going on?

interceptAtEnd captures a function which will be run once after the other middleware has been applied. It intercepts 2 possible users: toConfig (for webpack config) and get (e.g. for jest middleware reading babel config). It works by monkey-patching various layers of the API (and not in a performant way; this will keep building-up wrappers each time it's used. I didn't want to focus on optimising this hack)

What does this mean?

Adding typescript requires some config before any other module is loaded (supported extensions), and some config after the main compilation module is loaded (add plugins, etc.). That's why I needed ...Before and ...After before. With this interceptor, they can be rolled into one module which should be listed first in the use section:

module.exports = {
  use: [
    typescript(),
    airbnb(),
    typescriptLint(),
    jest(),
    node(),
  ],
};

The typescriptLint also makes use of this helper function, which means it can appear anywhere (i.e. before or after the main linting module is loaded).

Obviously I'm not advocating for this hack, but I think adding proper support for this kind of thing would benefit a lot of the existing modules here. Something like tap but tapAtEnd, with a formal API for saying "get me the final resolved config for this module" (like get but would run then clear the tapAtEnd-registered functions if present).

Also the intent here would be to create 2 modules: typescript and typescriptLint. They're combined here for simplicity, but can now be broken apart easily (which reduces the number of dependencies needed if just using typescript without any linting).


None of this removes the need for tsconfig.json, which I think is a hard requirement of any typescript project, especially if you want proper editor support. On that note, I also discovered that for better babel compatibility, "module": "esnext" and "resolveJsonModule": true should be set there as well as the options listed before.

@davidje13
Copy link
Contributor Author

If anybody wants to play with this, I have created a few packages which encapsulate the modules:

https://github.com/davidje13/neutrino-typescript
https://github.com/davidje13/neutrino-typescript-eslint

example / playground project: https://github.com/davidje13/neutrino-typescript-example

I haven't tested many situations so this is probably rather brittle.

@davidje13
Copy link
Contributor Author

davidje13 commented Aug 4, 2019

More testing with this has revealed that when building a library, the configuration above works fine unless you want to include generated .d.ts files (which is usually desirable for this kind of setup)

Sadly, generating .d.ts files isn't supported by the webpack integration (not sure if this is planned), and due to microsoft/TypeScript#29490 it requires a separate tsconfig.json file to make it all work:

tsconfig-decl.json:

{
  "extends": "./tsconfig",
  "compilerOptions": {
    "declaration": true,
    "noEmit": false,
    "emitDeclarationOnly": true,
    "declarationDir": "build",
    "isolatedModules": false
  }
}

package.json build script:

webpack --mode production && tsc -p tsconfig-decl.json

Yes that's performing 2 separate builds; one via webpack to generate the code, and one via tsc to generate the declaration files. Also note that the tsconfig-decl file needs to know the output directory so that it can put the declarations in the right place.

This is overall pretty ugly and I can't think of ways to abstract it in neutrino short of generating these files on-the-fly. When the linked bug is resolved, things will be a little easier (it won't need to be 2 separate files), but it still means more dynamic config living inside the static tsconfig.json file due to the wontfix microsoft/TypeScript#25271.

@elpddev
Copy link

elpddev commented Oct 22, 2019

@davidje13 Thanks for the hard work. Allowed me to get some insight before tackling it in our project.

Regarding the type declaration emittion, I do this by activating the typescript complier api after webpack has finished compiling. I detect webpack is done compiling using hooks. This allows to live without the special tsconfig.

webpack hook

neutrino.config.plugin('emitTypes').use({                    
┊ apply: compiler => {                                       
┊ ┊ compiler.hooks.afterEmit.tap('AfterEmitPlugin', () => {  
┊ ┊ ┊ generateTypes();                                       
┊ ┊ });                                                      
┊ },                                                         
});                                                          

compiler api generate type

const ts = require('typescript');                                   
                                                                    
exports.generateTypes = () => {                                     
  const options = ts.getDefaultCompilerOptions();                   
                                                                    
  const newOptions = {                                              
  ┊ ...options,                                                     
  ┊ declaration: true,                                              
  ┊ declarationDir: './build',                                      
  ┊ declarationMap: true,                                           
  ┊ emitDeclarationOnly: true,                                      
  ┊ outfile: 'build/index.d.ts',                                    
  };                                                                
                                                                    
  const program = ts.createProgram(['./src/index.ts'], newOptions); 
  const results = program.emit();                                   
                                                                    
  return results;                                                   
};                                                                  

@davidje13
Copy link
Contributor Author

davidje13 commented Oct 26, 2019

@elpddev that's a nice way of handling it. Simplifies the necessary package.json script too.

I'd be interested to hear from a core maintainer whether that violates v9's philosophy of not running the tooling directly, and just configuring external tools. Since it leaves tsc working with the regular build, and rolls this in as part of webpack's build, I'd be inclined to say it's fine. Really it should be part of the typescript/webpack integration (though I don't know which specific project that PR would be for)

Since it moves the config into javascript land, I'm guessing a lot of those path constants could be computed? I might have a go at converting the experimental middleware I wrote to work this way if I get some time (or feel free to make a PR yourself).

@davidje13
Copy link
Contributor Author

@elpddev I updated my experimental middleware (https://github.com/davidje13/neutrino-typescript) with your suggestion. It avoids hard-coded file paths by checking the neutrino options. Can you check if it works for you? I'm trying to make it work with a broad range of project types, but I only have a limited set of my own to test with. Specifically, I've found that specifying outFile makes a file which is not the correct format (complains with not a module when used). I notice that you used outfile which I think is being ignored because of the lowercase f, but if not then I guess there's some other difference in how you're using this.

The readme has full details, but should just need typescript({ declaration: true }) in your .neutrinorc.js.

@elpddev
Copy link

elpddev commented Oct 28, 2019

@davidje13 I will try to test it when I can. You are correct regarding the outfile letter case
https://github.com/microsoft/TypeScript/blob/61cb06ce401ea9d7ace005ea1c770517d4497427/src/harness/compiler.ts#L87.
I did not need one file so I did not notice it. I removed it also now from my code.

@sep2
Copy link

sep2 commented Nov 30, 2019

There is an issue that this preset cannot work with preset-react davidje13/neutrino-typescript#1

@tom-mckinney
Copy link

Are there any updates on the current state of this proposal? I would love to use typescript with neutrino 🙂

@edmorley
Copy link
Member

I'm not actively working on Neutrino at the moment, since I no longer use it day to day for work. As such any new feature work will need to come from external contributions. For a feature like this which is quite nuanced (if only because of the half-dozen different ways one can configure a TypeScript webpack project), asking someone to open a PR is a bit of a hard ask, since there likely needs to be more discussion/agreement as to choices before a PR can be reviewed. As such I'm not really sure where this leaves us. If someone wants to write up the pros/cons of different plugins/loaders/... and champion the effort then I'll do my best to read through and assist over the coming months.

Also, the conversation is now kinda split between here and #1269 perhaps we should recombine them?

@davidje13
Copy link
Contributor Author

I encourage anybody who wants this functionality to try out my experimental projects which contain the suggestions in this thread, and report any issues/limitations/improvements. If it gets to the point where it supports a wide enough range of use cases and has little enough weirdness, I'll make a PR:

I'm actively using these experimental plugins in several of my own projects and some other people have used them too. Due to typescript not supporting dynamic configuration, it's not quite as simple to use as most plugins, but it's still workable.

@relnod
Copy link

relnod commented Apr 10, 2020

Because I had a lot of trouble setting up neutrinojs with preact and typescript I created an example using the repositories from @davidje13

https://github.com/relnod/neutrino-preact-typescript-example

It would have been nice, if the setup were simple and and straightforward. Ideally I could shoose typescript during the project setup just like choosing preact instead of react.

@davidje13
Copy link
Contributor Author

@relnod hopefully eventually these modules will be brought into core neutrino and the setup script will be extended to work with them, but for now they're still quite experimental.

By coincidence, I'm also playing around with preact+typescript at the moment and I've just fixed the .tsx file handling. If you update your dependency to 1.0.11 and add "jsxFactory": "h" to your tsconfig.json, you should find that you don't need /** @jsx h */ at the top of all your files any more.

@davidje13
Copy link
Contributor Author

Now on NPM! (seemed stable enough to graduate from being a GitHub dependency)


I've just done a bit of reworking of neutrinojs-typescript so that it can (optionally) generate the tsconfig.json file. This makes setting up a project a little easier. It's backwards-compatible and easy enough to migrate to the new way. See the readme for details.

TypeScript still doesn't allow dynamic configuration (and in all likelihood never will), but I added an executable rewrite-tsconfig which can be invoked in npm scripts to auto-generate a tsconfig.json file. It uses a new neutrino().tsconfig() target.

This means custom configuration should now live in .neutrinorc.js which makes it consistent with other neutrino modules. I've tried to auto-generate as much of the config as possible, so customisations should be limited to choice of parsing strictness, etc.

Examples have been updated: neutrino-typescript-example / neutrino-typescript-react-example

I recall discussing auto-generating tsconfig.json before and it wasn't a well-liked solution, but after using this for some time I'm thinking it's the best option out of a bunch of bad options. @eliperelman / @edmorley it would be good to get your views on whether this approach fits with neutrino.

@edmorley edmorley closed this as completed Feb 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

7 participants