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

Add Console types #452

Closed
wants to merge 2 commits into from
Closed

Add Console types #452

wants to merge 2 commits into from

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Apr 24, 2018

This introduces namespace types.

This removes:

and adds:

  • .countReset()
  • .timeLog()

@@ -2049,6 +2010,26 @@ declare var XMLHttpRequestUpload: {
new(): XMLHttpRequestUpload;
};

declare namespace console {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is going to be a breaking change. i will need to run the tests against definitelytyped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would type Console = typeof console; mitigate the compat problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

My concern is that augmentation to Console are now ignored. making it a type makes it an error. but does not address the issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My quick search on DefinitelyTyped (with a search query interface Console ) says only arangodb augments the existing Console definition, other types including Node.js and react-native have their own ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

extends Console also only shows core-js.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The arangodb type is not really about augmenting existing browser console but about typing their own console module: https://github.com/arangodb/arangodb/blob/16a1fabf2057fb1da56a7abd44bd8e27ab1bead5/js/common/bootstrap/modules/console.js

core-js dropped their core.log that was using extends Console 4 years ago. (DefinitelyTyped/DefinitelyTyped#36100)

@saschanaz saschanaz force-pushed the console branch 2 times, most recently from 1e91c8a to 4c5bee4 Compare June 11, 2019 04:56
@saschanaz
Copy link
Contributor Author

@sandersn, I just refreshed this PR, could you review this?

Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Some additional questions. I'm retesting this PR on Definitely Typed.


declare var Console: {
prototype: Console;
new(): Console;
Copy link
Member

Choose a reason for hiding this comment

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

When was this valid, if ever? (At least, I tested it on V8 and didn't get anything; I don't have Firefox installed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No Console public constructor has ever existed, at least since the spec was first published in 2014.

IE11 somehow has it, though.

@@ -18878,6 +18840,29 @@ declare var webkitRTCPeerConnection: {

declare type EventListenerOrEventListenerObject = EventListener | EventListenerObject;

declare namespace console {
Copy link
Member

Choose a reason for hiding this comment

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

I'll re-test @mhegazy's question. Here's how I'll do it in case you want to try it yourself.

$ git checkout console
$ npm test
$ cp baselines/* ~/ts/src/lib
$ cd ~/ts
$ gulp
$ cd ~/dtslint-runner
$ npm run build
$ node bin/index.js --localTs ~/ts/built/local

You need clones of Typescript, dtslint-runner and DefinitelyTyped to make this work.

@@ -3519,40 +3519,6 @@ interface ConcatParams extends Algorithm {
publicInfo?: Uint8Array;
}

/** Provides access to the browser's debugging console (e.g. the Web Console in Firefox). The specifics of how it works varies from browser to browser, but there is a de facto set of features that are typically provided. */
interface Console {
memory: any;
Copy link
Member

Choose a reason for hiding this comment

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

The following properties are now gone:

  • memory
  • exception
  • [mark]timeline[End]

Do you know who this is likely to break? People with dependencies on old versions of IE? Somebody else?

Copy link
Contributor Author

@saschanaz saschanaz Jun 12, 2019

Choose a reason for hiding this comment

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

Seems that I have to keep .memory and .exception for now because they are in Edge and Firefox although not in spec. .markTimeline is only in Edge (and not in IE11) in non-functional state.

@sandersn
Copy link
Member

The following packages had errors: angular-meteor, apollo-upload-client, atom-mocha-test-runner, atom, backlog-js, baconjs, blessed, blob-stream, blob-to-buffer, body-parser, bytebuffer, cacheable-request, carlo, cassanknex, chai-enzyme, chai-webdriverio, cheerio, clean-css, consul, core-js, cron, depd, documentdb, dom-inputevent, dom-to-image, dropboxjs, electron-clipboard-extended, electron-json-storage, electron-notify, electron-notifications, electron-spellchecker, electron-window-state, engine.io-client, dojo, enzyme, enzyme-adapter-react-16, enzyme-adapter-react-15, enzyme-to-json, event-to-promise, evernote, expect-puppeteer, feathersjs__authentication-client, frisby, got, got/v8, gulp, gulp-cheerio, html5-to-pdf, htmlparser2, imperium, jasmine-enzyme, jest-environment-puppeteer, jsdom, jsdom/v2, jszip, kefir, koa-compose, kue, lasso, leadfoot, line-by-line, listr, lobibox, magnet-uri, maker.js, marko, menubar, meteor-collection-hooks, meteor-astronomy, meteor-persistent-session, meteor-jboulhous-dev, meteor-prime8consulting-oauth2, meteor-publish-composite, meteor-roles, meteor, mozilla-readability, multy, n3, multer-s3, next-redux-wrapper, next-redux-saga, nightmare, node/v0, node, node/v6, node/v11, node/v7, node/v10, node/v8, node/v9, node-persist, nodemailer-ses-transport, nookies, nw.gui, nw.js, openfin, openfin/v37, openfin/v39, openfin/v34, openpgp, orchestrator, papaparse, parse, parse/v1, parse-torrent, parse-mockdb, parse-torrent-file, pouch-redux-middleware, pouchdb, pouchdb-adapter-fruitdown, pouchdb-adapter-http, pouchdb-adapter-idb, pouchdb-adapter-leveldb, pouchdb-adapter-localstorage, pouchdb-adapter-node-websql, pouchdb-adapter-memory, pouchdb-browser, pouchdb-adapter-websql, pouchdb-core, pouchdb-find, pouchdb-http, pouchdb-mapreduce, pouchdb-live-find, pouchdb-replication, pouchdb-node, pouchdb-upsert, promised-temp, promisify-supertest, protractor-browser-logs, puppeteer/v0, puppeteer, puppeteer-core, qrcode, rasha, react-bootstrap-table/v2, react-csv, react-hot-loader, react-router, react-timeout, redux-storage, roads, roads-server, sass-webpack-plugin, s3-download-stream, s3-upload-stream, shapefile, sharedb, simple-cw-node, simple-peer, snoowrap, socket.io-parser, sortablejs, spdy, sql.js, statsd-client, sqs-producer, sqs-consumer, stompjs, storybook__addon-a11y, styled-components/v3, stylus, superagent, superagent/v2, superagent-bunyan, superagent-prefix, superagent-no-cache, supertest, supertest-as-promised, tesseract.js, text-buffer, thrift, touch, vast-client, webappsec-credential-management, webgl2, webtorrent, whatwg-streams, wonder.js, xmlpoke, zipkin-context-cls, zipkin-transport-http, zui.

I'm going to look into some of them; a lot seemed quite similar.

@sandersn
Copy link
Member

The majority of the errors are from @types/node, which needs to switch to a namespace as well. After my follow-up run finishes, I'll post the list of @types that still fail with some errors.

@sandersn
Copy link
Member

Here's the errors after fixing node. I didn't fix node/v* so there are errors from there. Besides that, it looks like a few other packages have their own declarations of console.

errors.txt

@saschanaz
Copy link
Contributor Author

Some errors are from the removed window.console, which can probably be fixed via globalThis.

@orta
Copy link
Contributor

orta commented Feb 25, 2020

👋 - I took a look over this PR, I'd love to see Console changes move to the IDL.

Now that globalThis is available , is it feasible to get this PR to a mergeable state? If yes, we'll try re-run the DT checks and see if node is still broken.

@saschanaz
Copy link
Contributor Author

IMO ready to be reviewed again 👍

@sandersn
Copy link
Member

DT still has 199 failures. Almost all of them are because node still has a global declare var console: Console that will need to become a namespace (and of course add the new properties). I'm not sure how to make that happen without breaking old versions of Typescript though:

declare var console: Console
// and
declare namespace console {
  export function f();
}

Are not going to merge, but that's what you'll have when mixing an old node with a new Typescript, or vice versa.

Judging from the other errors:

  • meteor tries to declare var console
  • jsdom might try to extend the Console type.
  • zipkin refers to the Console type, but I'm not sure if it tries to extend it.
  • External packages @jest/console and meteor-typings do the same.

So the next step is to make sure that @types/node can be used with these new dom types, and, after being changed, still works with the old ones too. There might be an obvious fix that I'm missing, but I haven't seen it yet.

@sandersn
Copy link
Member

sandersn commented Mar 4, 2020

As-is, I think there are too many breaks to take this. Unfortunately, I think that the IDL-generated namespace is not going to work with @types/node, at least not without some solution I haven’t thought of before.

I’m going to close this and recommend adding the new functions to overridingTypes.json. @saschanaz if you have an idea how to make @types/node compatibility work, let me know and we can restart this effort.

@sandersn sandersn closed this Mar 4, 2020
@saschanaz
Copy link
Contributor Author

Well, I don't have any other workaround in mind, so how about we tweak the script a little bit to generate an interface instead, special casing console?

@sandersn
Copy link
Member

sandersn commented Mar 5, 2020

I thought about that, but that seems hardly different from entries in overridingTypes.json. And the code to support overridingTypes.json is already written.

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.

4 participants