broccolijs / broccoli

Browser compilation library – an asset pipeline for applications that run in the browser
https://broccoli.build
MIT License
3.33k stars 217 forks source link

Add Brocfile.ts TypeScript support #390

Closed oligriffiths closed 5 years ago

oligriffiths commented 5 years ago

This PR adds support for loading a typescript Brocfile and running it through ts-node. If one provides a Brocfile.ts, install ts-node and typescript, it will be auto picked up and run through ts-node. An optional tsconfig.json can exist alongside the Brocfile.ts, however defaults taken from https://github.com/stefanpenner/fs-tree-diff/pull/89/files#diff-e5e546dd2eb0351f813d63d1b39dbc48 are provided

I think I've provided a type definition for the BrocfileOptions, but I may have gotten this wrong, plz inform. Tests added.

oligriffiths commented 5 years ago

@stefanpenner @rwjblue have you had a chance to check this out? Am I on the right path?

oligriffiths commented 5 years ago

@chriskrycho Would you be interested in having a look at this given your work on ember-typescript?

oligriffiths commented 5 years ago

@mike-north Would you be interested in having a look at this given your work on ember-typescript?

oligriffiths commented 5 years ago

Ok, so here's an update. I've removed the ts-node and typescript dependencies, and now throw an exception if they're not installed, prompting the user to install them. However, I have some questions on how we want ts-node to work by default, and if we should provide default options.

  1. Should type definitions be defined for imported modules? For example, doing import merge from 'broccoli-merge-trees', TS complains that there's no type information for this, and it's defaulting to any type. noImplicitAny: false is required to fix this. The downside of requiring this is that no broccoli plugins currently have type definitions, so this would be painful for early adopters.
  2. How should importing from CommonJS modules work? This is affected by esModuleInterop: true config option if we want it to work out of the box for import merge from 'broccoli-merge-trees'
  3. Should we allow importing a JS file from a TS file? If so, allowJs: true is required.
  4. if allowJs: true is used, the tests fail when there is more than 1 test that loads a TS brocfile (a single one passes, multiple fail): https://travis-ci.org/broccolijs/broccoli/jobs/502880617#L1065
     TSError: ⨯ Unable to compile TypeScript:
    Brocfile.ts(2,24): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
    Brocfile.ts(2,32): error TS2683: 'this' implicitly has type 'any' because it does not have a type annotation.
    Brocfile.ts(5,23): error TS2304: Cannot find name 'exports'.
    Brocfile.ts(6,43): error TS2580: Cannot find name 'require'. Do you need to install type definitions for node? Try `npm i @types/node` and then add `node` to the types field in your tsconfig.
    Brocfile.ts(8,1): error TS2304: Cannot find name 'exports'.

😞

mike-north commented 5 years ago

Should type definitions be defined for imported modules? For example, doing import merge from 'broccoli-merge-trees', TS complains that there's no type information for this, and it's defaulting to any type. noImplicitAny: false is required to fix this. The downside of requiring this is that no broccoli plugins currently have type definitions, so this would be painful for early adopters.

this is a pretty general issue having to do with typescript. Consumers should either define their own types locally, PR types into the dependency (through a declaration file or a JS -> TS conversion) or add a package to DefinitelyTyped

How should importing from CommonJS modules work? This is affected by esModuleInterop: true config option if we want it to work out of the box for import merge from 'broccoli-merge-trees'

the way to do this without forcing consumers to set this flag is

// import
import foo = require('foo');

// export
export = 'foo';
oligriffiths commented 5 years ago

Thank you to everyone who contributed feedback on this PR 🎉