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

Adding iterators to DOM interfaces #3393

Merged
merged 2 commits into from
Jun 9, 2015
Merged

Conversation

saschanaz
Copy link
Contributor

Fixing #2695, or at least #3381.

I added dom.es6.d.ts to do this. Not all interfaces listed in #2695 are covered because I couldn't find specs for them. I need some help for this. Spec list: saschanaz#1

I thought I have to add some tests as #3306 did but two things blocked me:

  1. There is no other tests for DOM interfaces in tests/cases/compiler. Is a test set only required for language features?
  2. I couldn't understand how a .symbols file can be written. How can I get the numbers to write something like >result.push : Symbol(Array.push, Decl(lib.d.ts, 1016, 29))?

Pinging @mhegazy to get some help.

- [ ] Add tests.

@danquirk
Copy link
Member

danquirk commented Jun 5, 2015

re:1 this is because in general we don't want compiler tests to use the full lib.d.ts as it incurs a significant performance overhead that the tests don't need. Whatever type relationship with DOM elements originally exposed a bug can be rewritten with an equivalent type hierarchy that doesn't need all the DOM types. We generally don't write tests that simply exist to call a DOM method and verify its API hasn't change. The lib.d.ts APIs reflect reality as they're generated from specs (ideally), so all such tests would do is incur an additional cost each time we need to regenerate/fix lib.d.ts as the DOM and other web APIs evolve.

@@ -145,11 +145,12 @@ var harnessSources = [
var librarySourceMap = [
{ target: "lib.core.d.ts", sources: ["core.d.ts"] },
{ target: "lib.dom.d.ts", sources: ["importcore.d.ts", "extensions.d.ts", "intl.d.ts", "dom.generated.d.ts"], },
{ target: "lib.dom.es6.d.ts", sources: ["importcore.d.ts", "es6.d.ts", "intl.d.ts", "dom.generated.d.ts", "dom.es6.d.ts"] },
Copy link
Contributor

Choose a reason for hiding this comment

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

i do not think you need this. the change to add the extensions to lib.es6.d.ts should be sufficient.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2015

as for 2. the .symbols is generated by the tests, it walks the tree, and queries the checker for symbols for each identifier, and then records that. this allows us to catch changes to the compiler internal state that may not have manifestation in the user code.

To update these files, I would just run the tests, look at the diff, if it is something you expect (i.e. changing position because you added a new declaration for instance) run jake baseline-accept and that would make the generated file the new baseline.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2015

I believe the change looks good. i do not think you need the lib.dom.es6.d.ts file, at least at this point, we have not found a way to expose lib.dom.d.ts anyways.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 5, 2015

As for what other dom types should be iterable.. i really do not know. let me ask someone on the IE team, and get back to you about this. though i do not think we need to block this change on it.

@saschanaz
Copy link
Contributor Author

Thanks for all your helps! No need to write a test then. I just removed lib.dom.es6.d.ts based on the feedback.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 9, 2015

Thanks. and sorry for the delay.

mhegazy added a commit that referenced this pull request Jun 9, 2015
Adding iterators to DOM interfaces
@mhegazy mhegazy merged commit 29afea3 into microsoft:master Jun 9, 2015
@saschanaz saschanaz deleted the es6dom branch August 28, 2015 11:10
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants