Skip to content

feat(): add typings to the blueprints. #175

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
Feb 9, 2016
Merged

Conversation

hansl
Copy link
Contributor

@hansl hansl commented Feb 4, 2016

Simply add typings to the blueprints, and use the browser ones in tsconfig.

This should get the upgrade to angular beta.2 unblocked.

"rootDir": ".",
"sourceMap": true,
"sourceRoot": "/",
"target": "es5"
}
},
"filesGlob": [
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not nice. Only atom understands this property, not tsc.

Your options are

  • use exclude, meaning all .ts and .d.ts files in this directory and children
  • use files =[] which means you need to specify the application entry point(s) as well as the typings/browser.d.ts file (or the typings/main.d.ts file if targetting node runtime)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, replaced with an exclude for the main file (we only need browsers AFAIK).

@alexeagle
Copy link
Contributor

LGTM

}
},
"exclude": [
"../typings/main.d.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

Excluding the file means the typings we have will never be accessible.

This is the problem I ran into: typescript allows you to either list all files, or exclude some of them. To use exclude you'd need typings to be installed on the src/ folder, and exclude the ones you don't want.

Edit: as @alexeagle said, this needs to have the main typings file and the app entry point. There were duplicate definitions errors because in angular2 beta.0 the typings for jasmine are included. In beta.2 there are no duplicate errors.

  "files": [
    "../typings/browser.d.ts",
    "app.ts"
  ]

Copy link
Contributor

Choose a reason for hiding this comment

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

On the other hand, beta.2 breaks ng serve for the same reason, so... a bit of a catch-22 here. You can test this by just upgrading npm angular2 on the test project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot use files because every unittest is its own entry point.

@hansl
Copy link
Contributor Author

hansl commented Feb 4, 2016

I'm going to try to upgrade my local app to beta.2 and see what happens. BRB.

@hansl
Copy link
Contributor Author

hansl commented Feb 5, 2016

@alexeagle @filipesilva @IgorMinar

Adding these typings work, when we pass in rootFilePaths to the broccoli-typescript. It's a custom property that is used by the TypeScript language service, defined in the ServiceHost. Basically we append our list of TS files to that property. rootFilePaths isn't standard though, in the official TypeScript host it's named getScriptFileNames.

After looking deeply into this (and it also gazed back into me, Nietzsche-style), I see a few problems with our current scripts in angular-cli:

  • nothing is typescript, but the source of the files is definitely TypeScript. We should replace our JS files by their TS sources and output the proper JS on prepublish. That's not much work I'll see if I can fit this into my schedule.
  • moving the typings.json to src/ isn't as convenient as it looks. It removes the use of process.cwd() from the angular broccoli class, but you need to cd src everytime you use typings, which isn't great.

Finally, I added a move to beta.3 in here, but I will remove it before I submit. The reason is we need this change in our npm package before we upgrade to beta.3. So unless you guys tell me it's okay to have a few days (until next release) where apps generated from master do not work (because the npm package isn't published), then I'll undo the upgrade. @filipesilva will be happy to know this is super straightforward though; just change the scaffold dependency up to beta.3 and the zone to .11 and it's good to go as is.

If you want to use this PR as a local test; once the app is generated you need to replace the node_modules/ version of angular-cli with your local copy. Basically, do the following shell commands:

./angular-cli/bin/ng new foo
cp -rf ./angular-cli foo/node_modules/
cd foo/
ng build  # will work

Please note that the tsc will not work unless you add all the entry points to the tsconfig.json files key. The problem is each spec file is its own entry point, so this would become unmanageable.

With that, I'm taking a break. Please take a look at your convenience.

@filipesilva
Copy link
Contributor

Whoa @hansl , great work! I can't follow up on this until next wednesday as I'm just leaving for a vacations, but there's enough info here for someone else to merge for now.

Again, thanks for looking into this, it was driving me nuts.

@hansl hansl force-pushed the typings branch 3 times, most recently from c3c7c19 to ebbd5bb Compare February 8, 2016 16:53
@filipesilva
Copy link
Contributor

lgtm 👍

@hansl hansl merged commit e238e32 into angular:master Feb 9, 2016
@hansl hansl deleted the typings branch February 9, 2016 22:02
@hansl hansl restored the typings branch February 10, 2016 20:24
@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 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants