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

feat: expose Console on global object [don't merge] #2856

Closed
wants to merge 3 commits into from

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Sep 3, 2019

It's useful to export Console from Deno object. Obvious example would be creating "capturing" console for testing module. Unfortunately I hit problem with types in TS which I can't figure on my own.

Currently experiencing this problem:

const fakeConsole = new Deno.Console(fakePrint);
window.console = fakeConsole;
error TS2741: Property '[isConsoleInstance]' is missing in type 'Deno.Console' but required in type 'consoleTypes.Console'.

 file:///Users/biwanczuk/dev/deno_std/testing/mod.ts:63:3

63   window.console = fakeConsole;
     ~~~~~~~~~~~~~~

  '[isConsoleInstance]' is declared here.

     $asset$/lib.deno_runtime.d.ts:2037:5

    2037     [isConsoleInstance]: boolean;
             ~~~~~~~~~~~~~~~~~~~

From lib.deno_runtime.d.ts:

declare interface Window {
   ...
   console: consoleTypes.Console;
   ...
}

Inside Console there is isConsoleInstance which is a unique symbol. This causes that Deno.Console.isConsoleInstance is a different symbol than consoleTypes.Console.isConsoleInstance.

I believe the problem lies in a way we generated our declaration file, but I'm not sure. Calling @kitsonk for help on this one.

@bartlomieju bartlomieju changed the title [WIPfeat: export Console [WIP] feat: export Console Sep 3, 2019
@ry
Copy link
Member

ry commented Sep 3, 2019

Can you add a test of some sort?

@bartlomieju
Copy link
Member Author

I added the test case it fails with error described above.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 4, 2019

Ah, yeah, the unique symbol thing... this recently crept up in the TypeScript repo, I can't remember the specific issue number, but TypeScript always emits unique for symbols, though not all symbols are unique... Since we aren't generating it anymore, I would just remove the unique and see if it works. In the code, if we case we ever generate it again, I would simply set it to /** @internal */ so it doesn't leak out anymore.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 4, 2019

Also, hopefully in TypeScript 2.7 there will be support for a form of nominal typing which will use symbol branding, so we would likely want to adjust to that in the future, because that is essentially what we are trying to express here is a nominal type versus a structural one.

@justjavac
Copy link
Contributor

isConsoleInstance is only used internally, should we remove it from the exported Console field. indentLevel too.

Even so, Type Deno.Console also can not assignable to type consoleTypes.Console, for 'private property printFunc'.

@kitsonk
Copy link
Contributor

kitsonk commented Sep 4, 2019

So the only easy way is to have just an interface then and guard based on that interface and keep the actual classes/types internal. This does highlight though that we do need to try to get back to a generated type library fairly quickly because trying to keep this in sync with code is going be not fun.

@bartlomieju
Copy link
Member Author

I think I figured it out - instead of exporting I exposed it on global object as it should be.

@kitsonk @justjavac please review

@bartlomieju bartlomieju changed the title [WIP] feat: export Console feat: expose Console on global object Sep 4, 2019
@nayeemrmn
Copy link
Collaborator

instead of exporting I exposed it on global object as it should be.

That breaks the browser compatibility rule.

@bartlomieju
Copy link
Member Author

instead of exporting I exposed it on global object as it should be.

That breaks the browser compatibility rule.

@nayeemrmn why is that? MDN tells that it's exposed on global (or Window in browsing scopes) which is de facto what I did.

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Sep 4, 2019

@bartlomieju Isn't this about the made up Console type/constructor? That link only talks about the console object (which is actually a namespace). We can make a convenience type for it, but that has to be in Deno.

@bartlomieju
Copy link
Member Author

@bartlomieju Isn't this about the made up Console type/constructor? That link only talks about the console object (which is actually a namespace). We can make a convenience type for it, but that has to be in Deno.

Alright, my bad, now I see what you mean. I'll think about it

@bartlomieju bartlomieju changed the title feat: expose Console on global object feat: expose Console on global object [don't merge] Sep 9, 2019
@bartlomieju
Copy link
Member Author

I'll revisit this PR after #2868

@bartlomieju bartlomieju closed this Oct 2, 2019
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.

5 participants