Skip to content

build: use typescript cli for compilation #2802

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

Merged
merged 1 commit into from
Jan 30, 2017

Conversation

devversion
Copy link
Member

@devversion devversion commented Jan 25, 2017

  • No longer use the gulp-typescript gulp package that tries to take care of several options like sourceMaps or declarations.
  • Don't generate declarations when serving the demo-app. (Slows down testing)

Advantages:

  • Use the Typescript compiler CLI to ensure that the NGC also works with our tsconfig.json files.
  • No longer makes our TypeScript build dependent to an external plugin (~with magic)
  • Fixes issues like invalid sourceMaps from gulp-typescript: build: fix sourcemaps for dev-app and e2e-app. #2753
  • Allows us to take use of all TypeScript features (e.g The sourceMaps option)
  • This should help for our planned tsconfig.json file reduction.

* No longer use the `gulp-typescript` gulp package that tries to take care of several options like `sourceMaps` or `declarations`.
* Don't generate declarations when serving the `demo-app`. (Slows down testing)

Advantages:

* Use the `Typescript` compiler CLI to ensure that the `NGC` also works with our `tsconfig.json` files.
* No longer makes our TypeScript build dependent to an external plugin (~with magic)
* Fixes issues like invalid sourceMaps from `gulp-typescript`: angular#2753
* Allows us to take use of all TypeScript features (e.g forbidden features in `gulp-typescript)
* This should help for our planned `tsconfig.json` file reduction.
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 25, 2017
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

If WebStorm is good, go ahead and add the "merge ready" label

"./demo-app-types.d.ts",
"./demo-app-module.ts",
"./system-config.ts",
"./main.ts"
Copy link
Member

Choose a reason for hiding this comment

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

In the past WebStorm had issues when you specified files where it would only provide completion / error checking for exactly these files and not their dependencies. Are you able to tell if that's still an issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just looked at it. The issue you described has been solved in Webstorm.

I just noticed that if you create a file (that is never referenced) Webstorm will complain for example about experimentalDecorator support. I don't consider this is as a big issue. What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably okay- we shouldn't have any dead code anyway.

Copy link
Member Author

@devversion devversion Jan 27, 2017

Choose a reason for hiding this comment

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

Yeah, also I think that it's not very typically for TS to not use a glob to walk through the files, instead TS walks through the chain of files.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 27, 2017
@kara kara merged commit 4a4261e into angular:master Jan 30, 2017
@devversion devversion deleted the build/use-typescript-cli branch January 31, 2017 09:57
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants