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

Re-enable type checking #8217

Closed
wants to merge 9 commits into from
Closed

Re-enable type checking #8217

wants to merge 9 commits into from

Conversation

ahocevar
Copy link
Member

Looks like the recent Closure Compiler update changed things so type checking the way we want it now requires compilation_level: 'ADVANCED'.

With this change, we're back at >2000 warnings.

@fredj, maybe you want to take a look at those? I've pushed this branch to openlayers if you want to add commits.

@ahocevar
Copy link
Member Author

ahocevar commented May 23, 2018

Looks like path types are broken again in the current version of Closure Compiler (see 2b1d897). Given the fact that this seems to change with every release, I don't feel right now like the decision to continue using Closure Compiler for type checking was right.

@fredj
Copy link
Member

fredj commented May 24, 2018

c1c9c77 deactivates extra checks made the NTI. With this the error count goes from 1238 to 387

@ahocevar
Copy link
Member Author

ahocevar commented Jun 5, 2018

@fredj Will you be able to continue working on this?

@fredj
Copy link
Member

fredj commented Jun 6, 2018

@ahocevar I'm not sure it's worthwhile to continue to enable the New Type Inference checks. What about changing the Closure Compiler configuration to have the same checks as the 4.x branch?

@ahocevar
Copy link
Member Author

ahocevar commented Jun 6, 2018

@fredj:

What about changing the Closure Compiler configuration to have the same checks as the 4.x branch?

Sounds good to me.

@fredj fredj force-pushed the closure-errors branch 2 times, most recently from 4506acc to ceaeb06 Compare June 8, 2018 15:01
@schmidtk
Copy link
Contributor

@ahocevar / @fredj: Is the long-term plan for OpenLayers 5.x to continue using the Closure Compiler for type checking?

I'm looking into necessary steps for migrating a fairly large project (OpenSphere) to ES6 and OL 5.x, while continuing to use the Closure Compiler for type checks and advanced optimizations. This seems like it should work if we use babel-plugin-jsdoc-closure to transform the JSDoc types for the compiler, but this approach would hinge on continued support for the Closure Compiler from OpenLayers.

@fredj fredj added this to the v5.0.0 milestone Jun 12, 2018
@ahocevar
Copy link
Member Author

ahocevar commented Jun 15, 2018

This is not a blocker for the 5.0.0 release. If it's not finished by the time we cut the release, we'll publish 5.0.0 without type checking.

After the release, we can experiment with typescript JSDoc annotations (see microsoft/TypeScript#5158 and e.g. webpack/webpack#7031) to replace the JSDoc/Closure type annotations we currently use.

If that turns out to work, we'll be able to use better tooling for the library, and Closure compilers users should still have a way to automatically transform these annotations to ones that Closure compiler understands -- similar to what https://npmjs.com/package/babel-transform-jsdoc-closure currently does.

@fredj
Copy link
Member

fredj commented Jun 18, 2018

ok, removing the 5.0.0 milestone

@fredj fredj removed this from the v5.0.0 milestone Jun 18, 2018
@fredj
Copy link
Member

fredj commented Jun 28, 2018

closure compiler will not be used to do the type-checking. closing

@fredj fredj closed this Jun 28, 2018
@fredj fredj deleted the closure-errors branch June 28, 2018 12:26
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.

3 participants