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

--emitDeclarationOnly flag to enable declarations only output #20735

Merged
merged 6 commits into from
Jan 25, 2018
Merged

--emitDeclarationOnly flag to enable declarations only output #20735

merged 6 commits into from
Jan 25, 2018

Conversation

nojvek
Copy link
Contributor

@nojvek nojvek commented Dec 16, 2017

@nojvek nojvek changed the title Adding noEmitJs flag to enable declarations only output Dec 16, 2017
if (options.noEmitJs && !options.declaration) {
programDiagnostics.add(createCompilerDiagnostic(Diagnostics.Option_0_cannot_be_specified_without_specifying_option_1, "noEmitJs", "declaration"));
}

Copy link
Member

Choose a reason for hiding this comment

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

you would also want to skip verifyEmitFilePath(emitFileNames.jsFilePath, emitFilesSeen); down below when noEmitJs is true

@nojvek
Copy link
Contributor Author

nojvek commented Jan 3, 2018

Kind ping regarding this @sheetalkamat

Should I be adding any specific devs from typescript team to this PR? What's the usual process before a PR lands in the repo ?

@@ -231,6 +231,13 @@ namespace ts {
category: Diagnostics.Basic_Options,
description: Diagnostics.Do_not_emit_outputs,
},
{
name: "noEmitJs",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the name should be --emitDeclarationsOnly instead.

name: "noEmitJs",
type: "boolean",
showInSimplifiedHelpView: true,
category: Diagnostics.Basic_Options,
Copy link
Contributor

Choose a reason for hiding this comment

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

i would say this belongs into Advanced_Options

Copy link
Contributor Author

@nojvek nojvek Jan 21, 2018

Choose a reason for hiding this comment

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

Advanced_options don't show up as part of --help or in default tsc --init. Doesn't it make sense that the option showed up there? I plan to enable allowJs + emitDeclarationsOnly in another PR which would help with lots of npm projects to auto-generate jsdoc to .d.ts declarations.

Puppeteer is one good example

@@ -2776,6 +2776,10 @@
"category": "Message",
"code": 6013
},
"Do not emit js outputs.": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Only emit declaration files.

@@ -980,7 +980,7 @@ namespace ts.server {
};

const ioSession = new IOSession(options);
process.on("uncaughtException", err => {
process.on("uncaughtException", (err: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it was failing compilation when doing a “gulp build” with an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

should work now. that was a @types/node issue.

kevlened added a commit to kevlened/str2buf that referenced this pull request Jan 6, 2018
@niieani
Copy link

niieani commented Jan 17, 2018

@nojvek awesome job so far! The Puppeteer community would love some progress on this! ❤️

@nojvek
Copy link
Contributor Author

nojvek commented Jan 17, 2018 via email

@nojvek nojvek changed the title noEmitJs flag to enable declarations only output Jan 23, 2018
@nojvek
Copy link
Contributor Author

nojvek commented Jan 23, 2018

@mhegazy want to take another pass ?

@niieani - Unfortunately this PR doesn't enable emitting declarations from a JS project. I'll start work on it as soon as this PR lands.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 24, 2018

@DanielRosenwasser and @sheetalkamat any concerns?

@mhegazy mhegazy merged commit afc588e into microsoft:master Jan 25, 2018
@niieani
Copy link

niieani commented Jan 26, 2018

Thank you @nojvek! Looking forward to the next PR 😊!

@nojvek
Copy link
Contributor Author

nojvek commented Jan 29, 2018

@mhegazy @sheetalkamat

// @declaration: true
// @emitDeclarationsOnly: true

For consistency with declaration: true flag, does it make sense to call this new flag emitDeclarationOnly ?

        declaration?: boolean;
        emitDeclarationsOnly?: boolean;
        declarationDir?: string;

It seems the pattern is to use singular *declaration*.

What do you think ?

I can send a rename PR

@mhegazy
Copy link
Contributor

mhegazy commented Jan 30, 2018

humm.. good point.. we can change it to EmitDeclarationOnly

@nojvek
Copy link
Contributor Author

nojvek commented Jan 30, 2018 via email

@mhegazy
Copy link
Contributor

mhegazy commented Feb 5, 2018

rename PR up in #21651

@mhegazy mhegazy changed the title --emitDeclarationsOnly flag to enable declarations only output Feb 5, 2018
mhegazy pushed a commit that referenced this pull request Feb 5, 2018
* Add emitOnlyDeclarations flag

* Fix name

* verifyOptions checking logic

* Passing tests

* doJsEmitBaseline

* Tests !!!
mhegazy added a commit that referenced this pull request Feb 6, 2018
* --emitDeclarationsOnly flag to enable declarations only output (#20735)

* Add emitOnlyDeclarations flag

* Fix name

* verifyOptions checking logic

* Passing tests

* doJsEmitBaseline

* Tests !!!

* Rename switch `--emitDeclarationsOnly` to `--emitDeclarationOnly` (#21651)

* Rename `--emitDeclarationsOnly` to `--renameDeclarationOnly`

* Rename test files
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
4 participants