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

[WIP] New type inference #4793

Closed
wants to merge 9 commits into from

Conversation

gberaudo
Copy link
Member

@gberaudo gberaudo commented Feb 4, 2016

Switch to the new type inference version of the compiler.
See https://github.com/google/closure-compiler/wiki/Using-NTI-%28new-type-inference%29

There are only 2 errors left:

ERR! compile /home/gberaudo/dev/ol3/src/ol/observable.js:22: ERROR - ol.Observable cannot extend this type; structs can only extend structs
ERR! compile ol.Observable = function() {
ERR! compile                 ^
ERR! compile 
ERR! compile 
ERR! compile /home/gberaudo/dev/ol3/src/ol/renderer/maprenderer.js:38: ERROR - ol.renderer.Map cannot extend this type; structs can only extend structs
ERR! compile ol.renderer.Map = function(container, map) {
ERR! compile                   ^
ERR! compile 
ERR! compile 
ERR! compile 2 error(s), 0
ERR! compile  warning(s)
ERR! compile 
ERR! Process exited with non-zero status, see log for more detail: 2 

TODO:

  • fix structs can only extend structs error;
  • fix publish.js

@gberaudo
Copy link
Member Author

gberaudo commented Feb 4, 2016

@fredj, if you want to have a look...

@@ -47,7 +47,7 @@ ol.events.Event.prototype.stopPropagation = function() {
/**
* @param {Event|ol.events.Event} evt Event
*/
ol.events.Event.stopPropagation = function(evt) {
ol.events.Event.prototype.stopPropagation = function(evt) {
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a mistake to me. There are still a number of places in the library that use the ol.events.Event.stopPropagation function. With this change, that will be undefined. I hope this causes at least a few test failures.

Not sure why you'd want it to be a prototype member. A name like ol.events.stopPropagation could make sense, but it doesn't need to be accessible from ol.events.Event instances.

Move olx.interaction before assignation.
Explicitly create objects.
Due to `ol.format.GML = ol.format.GML3;`, these are duplicates.
Breaks circular dependency.
Added to ol.Disposable and ol.events.EventTarget.
Avoid closure complaining about stopPropagation method not being present
in base class.
@gberaudo gberaudo force-pushed the new_type_inference branch 2 times, most recently from 94e26a8 to 35351aa Compare March 10, 2016 12:50
@gberaudo
Copy link
Member Author

The structs can only extend structs error seems to be gone after @tschaub's PR.
However there are lots of other errors, notably about nullable types.

@probins
Copy link
Contributor

probins commented Apr 29, 2016

@gberaudo I notice a couple of typedef-related changes in here. First, LRUCache looks like it should be changed to an interface, so I won't include this in my typedef PR. Second, in raster/operation.js you're adding goog.require('ol.raster.Pixel') even though that is only used in comments/typedefs. Were you getting some sort of error here that prompted you to add the goog.require()?

@gberaudo
Copy link
Member Author

@probins, thank you for caring about this PR.
I think it is better to ignore the changes here since some of them may be due to goog dependencies or the way the code is organized. Let's move the typedefs as discussed.

@gberaudo gberaudo closed this Apr 29, 2016
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.

None yet

3 participants