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

allowJs:true + declaration:true #21455

Closed
wants to merge 6 commits into from
Closed

allowJs:true + declaration:true #21455

wants to merge 6 commits into from

Conversation

nojvek
Copy link
Contributor

@nojvek nojvek commented Jan 29, 2018

  • There is an associated issue. Fixes Allow --declaration with --allowJs #7546
  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally
  • You've signed the CLA
  • There are new or updated unit tests validating the change

** TODO **

From previous PR: #15911

  • Fix commented out tsserver test. Not sure what the correct fix is
  • ES5 prototype style function definitions
function f() {
}
f.prototype.method = function() {}   // should probably emit a class?
  • @typedef {Object} SpecialType -> Interfaces
/**
 * @typedef {object} SpecialType1 - creates a new type named 'SpecialType'
 * @property {string} prop1 - a string property of SpecialType
 * @property {number} prop2 - a number property of SpecialType
 * @property {number=} prop3 - an optional number property of SpecialType
 */
/** @type {SpecialType1} */
var specialTypeObject1;
  • Recognize @private/@readonly -> private X / readonly X
  /** @private */
  _render() {...}

  /** @readonly */
  static observedAttributes() {...}

emolr added a commit to framejs/framejs that referenced this pull request Feb 8, 2018
…modules to prevent version mismatch. furthermore lit-html and preact is imported from js, this will give an compile error see this microsoft/TypeScript#21455. but it compiles fine, and makes it possible to use the renderers without any module bundling from the users side
// More comment`;
const configFileContentAfterComment = `
"compilerOptions": {
"allowJs": true,
Copy link
Member

Choose a reason for hiding this comment

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

you could change this to set { "inlineSourceMap": true, "sourceMap": true } to make sure the errors are reported on those lines in the config file.

@GirlBossRush
Copy link

This looks awesome! Is there an ETA to release?

@nojvek
Copy link
Contributor Author

nojvek commented Feb 28, 2018 via email

@DSchroer
Copy link

I did a quick test of this on a project I am developing for. It works great except that it automatically detects require("SOMETHING") as a module import even if the TypeScript module setting is set to none.

This is a weird quirk of the code I am dealing with but it would be nice to be able to disable module detection. Since in my case require is just another function.

@nojvek
Copy link
Contributor Author

nojvek commented Mar 1, 2018

Yeah there are lots of little quirks like that to work out.

But being able to auto-generate types for js projects is going to be amazing.

@weswigham
Copy link
Member

@nojvek Once #21930 is merged, I'd like to write a transformer for JS files that simply iterates over their exported symbols, gets their declared types, and uses a modified typeToTypeNode on them to generate declarations for all required symbols. It needs to be a very different process from our usual subtractive declaration emit.

@nojvek
Copy link
Contributor Author

nojvek commented Mar 1, 2018

ooo I like it. Do you have a guesstimate for the merge ?

@nojvek
Copy link
Contributor Author

nojvek commented Jun 21, 2018

Typescript 2.9 has introduced some nice goodies. I am going to take another stab at this.

Really liking what the improvements to jsdoc and codecompletion. Whoever did that. Thank you!

@ahocevar
Copy link

What needs to be done to finish this effort?

@nojvek
Copy link
Contributor Author

nojvek commented Aug 22, 2018

I basically just need to re-focus and get back into it. TSC has moved some pieces around so need to re-base and figure out the merge conflicts + tests.

Hopefully we don't end up bikeshedding again. Been trying to land this since last year now 😞

@weswigham
Copy link
Member

You probably want to start fresh rather than attempt a rebase - the declaration emit pipeline has entirely changed since this PR was last updated.

@RyanCavanaugh
Copy link
Member

@nojvek thanks for the work so far! I'm going to close this since it's going to require a substantial rebasing anyway. Looking forward to the next iteration.

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.

Allow --declaration with --allowJs
7 participants