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

update TypeScript dependancy #5485

Closed
wants to merge 1 commit into from

Conversation

chicoxyzzy
Copy link
Contributor

...and other deps except Babel

@facebook-github-bot
Copy link

@chicoxyzzy updated the pull request.

checker.emitFiles();
}
var emitResult = program.emit();
var errors = ts.getPreEmitDiagnostics(program).concat(emitResult.diagnostics);
Copy link
Member

Choose a reason for hiding this comment

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

Can you double (triple?) check this by putting some invalid type info into jest.d.ts? I want to make sure tests still fail in that case. I remember having some issues in the past when I tried to upgrade TS to 1.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, seems like tests pass in that case. I'll try to fix it soon. TypeScript's documentation on Compiler API is not very helpful :(

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I gave up and just left it. Since our use is so minimal, I think the only real win we would get from upgrading is being able to use JSX

Copy link
Collaborator

Choose a reason for hiding this comment

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

... which in an alternative universe might have prevented the spreadgate. 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@mhegazy says this should be fine.

@daukantas
Copy link

Thank you for #104.

Jorge

@zpao
Copy link
Member

zpao commented Feb 18, 2016

Are you interested in coming back to this @chicoxyzzy?

@chicoxyzzy
Copy link
Contributor Author

@zpao sure. I'll revisit this on Monday

@chicoxyzzy
Copy link
Contributor Author

@zpao did you meant that I should update TypeScript only? Because I see #5646 already

@chicoxyzzy
Copy link
Contributor Author

hmm.. #5646 is a bit outdated because current master has Jest 0.9.0-fb3 as a dependancy

@zpao
Copy link
Member

zpao commented Feb 26, 2016

Just update typescript. Unless you really want to take over #5646, I haven't had a chance to come back to it in the last couple weeks (just rebased and pushed though)

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

Ping @chicoxyzzy, would you mind changing this to only update TS? Thanks.

@gaearon
Copy link
Collaborator

gaearon commented Apr 8, 2016

@facebook-github-bot

b2ap3_large_istock_000011918649_small

@chicoxyzzy
Copy link
Contributor Author

@zpao why do we need our own compilerHost? I'd like to take update TS but I don't sure I understand all requirements. This information will be very helpful and I will be able to ask some help about compiler-related things in TS community.

@zpao
Copy link
Member

zpao commented Apr 13, 2016

I have no idea how the typescript stuff works, so no clue if the compilerHost is important.

There are 2 requirements:

  1. We can use TypeScript (duh 😀)
  2. Tests fail if the code doesn't typecheck. This is the case on master right now but did not hold true with your change (thus this discussion back in November: update TypeScript dependancy #5485 (comment))

@RReverser
Copy link
Contributor

@chicoxyzzy compilerHost is needed, in this particular case, in order to tell compiler API how to retrieve file contents by name etc., and in this particular case it's used in order to provide React libs from real files as well as contents being compiled in form of a "virtual file".

@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented May 2, 2016

I still have no time to work on this but AFAIK TypeScript 2 will be released in month or so as one guy from TypeScript team said here

@chicoxyzzy chicoxyzzy changed the title update Jest and TypeScript update TypeScript dependancy May 2, 2016
@DanielRosenwasser
Copy link
Contributor

I said we were thinking of a beta, not an official release, and no promises on the date. It won't hurt to bump to 1.8.

@chicoxyzzy
Copy link
Contributor Author

Yes I think it's not very hard to update ts-preprocessor.js. It's hard to realize what is the thing in tests the same thing for es6 classes and TypeScript test =)

@chicoxyzzy chicoxyzzy closed this May 2, 2016
@chicoxyzzy chicoxyzzy deleted the update_jest_and_ts branch May 2, 2016 22:46
@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented May 2, 2016

I just renamed branch locally and remotely. I'll create new PR when and if I'll get how to debug jest tests =)

@chicoxyzzy
Copy link
Contributor Author

ok I have some progress (I think). now I'm getting errors:

 FAIL  src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts
● Runtime Error
Error: undefined(9): Cannot find module 'React'.
undefined(10): Cannot find module 'ReactDOM'.

Trying to realise why resolving these modules has broken and how to fix that

@RReverser
Copy link
Contributor

@chicoxyzzy Because https://github.com/facebook/react/blob/master/src/isomorphic/modern/class/__tests__/ReactTypeScriptClass-test.ts#L10 imports modules React / ReactDOM while no definition of such modules is included for compilation. You need to change references and attach correct .d.ts (perhaps from typings).

@chicoxyzzy
Copy link
Contributor Author

chicoxyzzy commented May 3, 2016

@RReverser there are definitions here https://github.com/facebook/react/tree/master/src/isomorphic/modern/class

Definitions from typings are ambient and they are still for 0.14. I thought to update those definitions for 15.0 and add to React repo as mentioned here as another PR (@zpao is it ok?) because ambient definitions are hard to update and they are unofficial

You are right. I've add these references to ReactTypeScriptClass-test.ts and it works now.

/// <reference path="../React.d.ts" />
/// <reference path="../ReactDOM.d.ts" />

@chicoxyzzy
Copy link
Contributor Author

OK. Now tests pass even if I put invalid type info to jest definitions. Same as it was in November :/

@chicoxyzzy chicoxyzzy mentioned this pull request May 3, 2016
@chicoxyzzy
Copy link
Contributor Author

see #6686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants