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

noExternalResolve option for control over files automatically pulled into compilation, declarationFiles and declarationOuput option for batching d.ts files output #32

Closed
wants to merge 5 commits into from

Conversation

rob-bateman
Copy link

  • noExternalResolve option (defaults to false) to allow a gulp-typescript way of defining all files to include (without trying to automatically pull dependencies in)
  • declarationFiles option is a glob that defines d.ts files to be used in teh compilation process
  • declarationOuput allows a batched d.ts file to be output from the compiler, using ambient module declarations

Additional updates
introduced a fix for require paths to be compatible across dependencies in the node_modules folder (using absolute paths in the compiler)
fixed reporting errors thrown by the compiler not related to file errors
fixed file emit event to only include .ts file (leave out d.ts files)

@smrq
Copy link
Member

smrq commented Mar 13, 2015

noExternalResolve option (defaults to false) to allow a gulp-typescript way of defining all files to include (without trying to automatically pull dependencies in)

Solid! I don't see any issues with this.

declarationFiles option is a glob that defines d.ts files to be used in teh compilation process

Can't you just include those as entries to Browserify? Why do they need to be separated out?

declarationOuput allows a batched d.ts file to be output from the compiler, using ambient module declarations

Awesome, I'll be checking this guy for edge cases myself but good work!

Additional updates
introduced a fix for require paths to be compatible across dependencies in the node_modules folder (using absolute paths in the compiler)

Can you elaborate a little on what exactly this means? I think I understand your use-case with AwayJS, I just want to be sure I understand what this changes.

fixed reporting errors thrown by the compiler not related to file errors

Whoops! Thanks.

fixed file emit event to only include .ts file (leave out d.ts files)

This was intentional behavior, for compatibility with the Karma test runner (via karma-browserify). Was this a problem? I guess I could reevaluate whether it still needs to be there for Karma.

Final note, could you make sure the existing tests pass, and add tests for your added functionality?

Thanks for the contributions!

@rob-bateman
Copy link
Author

Answers inline:

declarationFiles option is a glob that defines d.ts files to be used in teh compilation process

Can't you just include those as entries to Browserify? Why do they need to be separated out?

browserify complained when i included .d.ts files in that way although i can't remember what the precise error was (something to do with reading imports) It would seem these files don't pass the initial dependency check, although if you can find a way to make it work i would totally agree its a better solution

Introduced a fix for require paths to be compatible across dependencies in the node_modules folder (using absolute paths in the compiler)

Can you elaborate a little on what exactly this means? I think I understand your use-case with AwayJS, I just want to be sure I understand what this changes.

All that was required here was to change your getRelativeFilename to getAbsoluteFilename and modules are then pickup up by tsc without issue. This doesn't affect the output at all, its simply using the same approach that worked for me in gulp-typescript

fixed file emit event to only include .ts file (leave out d.ts files)

This was intentional behavior, for compatibility with the Karma test runner (via karma-browserify). Was this a problem? I guess I could reevaluate whether it still needs to be there for Karma.

Actually yes, the d.ts files do not have to only include one module definition per file, and it was again throwing errors in the browserify loop. d.ts files are really only of use to the tsc, so I'm confused as to why you would want (or need) to pass them on to other plugins. Also, the setup you had wouldn't have ever passed on generated d.ts files, just the ones that you gave the compiler initially for compiling against. If you wanted to perform tests on these, you could do that completely outside of browserify

@rob-bateman
Copy link
Author

On the subject of tests, looks like the ones you have are not updated for the latest typescript compiler or latest browserify module or both. Either way, the problem are all in sourcemaps comparisons which is not an area these updates affect

@smrq
Copy link
Member

smrq commented Mar 16, 2015

As the CI build shows, the current tests do pass with the latest Browserify and tsc. Are you certain that you didn't make any updates that might affect the build output? Your change to prevent emitting of *.d.ts files should certainly necessitate some test updates, and I wouldn't be surprised if the absolute path change had some effect, as well.

@ccapndave
Copy link

I would love to use this (specifically declarationOutput but I see the PR hasn't been touched for a while. Does this still work, and is there any possibility of it getting merged?

@smrq
Copy link
Member

smrq commented Jul 9, 2015

There's just too much in this PR, and too much functionality broken. declarationOutput on its own would be nice but that functionality really needs a lot more tests to make sure that its output is valid.

@smrq smrq closed this Aug 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants