merge in definately typed TS definitions to parse project #388
Conversation
taking the work allready done https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/parse/index.d.ts and merge it into the project as per sugestions of https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html this will mean users of typescript & parse will have one less step to do (manually pull in types) and hopefully changes in the definitions themselves will stay closer to the actual thing
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@chrismatheson any updates on this? Looks like the types are a bit outdated already. For example, they're still referencing types for jquery and underscore which afaik are no longer used in the SDK? If you need any help to finish this maybe I can jump in and spend some time on it 👍 |
|
@agonbina i had exactly the same thought (that the typedefs were out of date). The i started to think about updating those shared typedefs i went down a hole of "well this should probably be a PR to the project itself as those maintainers would have a better understanding of the correctness" so i figure the smallest change was to pull in the current typedefs to this project 1st, then PR here to bring them up-to-date however i could see the benefit of doing it all at once, so if thats considered a pre-req of merging this change ill happily attack that & welcome all help :) |
|
Committing those files is cumberstone and the type definitions should probably distributed separately. I'm no expert in this, but it seems that the burden is on the developer to run those definitions for every PR, and it doesn't work well. We can think of a CI step that would, upon releases rebuild those. What do you think? |
|
Can you get us setup to publish to the |
|
@flovilmart admitidtly I've been quite lax on this one, but I'm still keen for it to be part of this project itself, a couple of reasons .... As part of the project there is no need to install two things which is nice for those of us using typescript. The definitions add no weight to production code (left out by both browserify/webpack style build & HTML script style builds) But also, separating the types from the project allows too easily for drift between the "real" world and the type defs, if the types are to be useful then anyone changing the api of the parse my. Should update the typedefs alongside that change (so keep the two thing together :) ) As for the "burden", yes this would be on the developer submitting any PR that changes API of the parse lib (happy side note that anything touching the typedefs is probably a major or minor semver change and this make it more visible) |
|
@chrismatheson What I suggest is that we automate this process but don't put the burden on the developer (which don't make sense to me) nor commit those files which are purely generative. What I recommend moving forward is to include it when releasing the package, the generation can occur at What do you think? |
| "browser": { | ||
| "react-native": false | ||
| }, | ||
| "dependencies": { | ||
| "@types/jquery": "^2.0.34", |
There was a problem hiding this comment.
those are dev dependencies.
| "../../index.d.ts", | ||
| "parse-tests.ts" | ||
| ] | ||
| } |
| @@ -53,11 +58,13 @@ | |||
| "gulp-uglify": "^1.4.0", | |||
| "jasmine-reporters": "~1.0.0", | |||
| "jest-cli": "^15.1.1", | |||
| "typescript": "^2.1.4", | |||
There was a problem hiding this comment.
this one is duplicated here.
| "vinyl-source-stream": "^1.1.0" | ||
| }, | ||
| "scripts": { | ||
| "build": "./build_releases.sh", | ||
| "release": "./build_releases.sh && npm publish", | ||
| "pretest": "tsc integration/types/index.ts --noEmit", |
There was a problem hiding this comment.
add a step to build index.d.ts why would you want tsc to be run on all machines and a requirement for all developers? I don't mind adding it to the CI. What's the benefit of running it as a pre-test?
| query.include("score"); | ||
| query.include(["score.team"]); | ||
|
|
||
| var testQuery = Parse.Query.or(query, query); |
| class TestCollection extends Parse.Collection<Object> { | ||
|
|
||
| constructor(models?: Parse.Object[]) { | ||
|
|
| var src = file.url(); | ||
|
|
||
| file.save().then( | ||
| () => { |
| // result | ||
| }); | ||
|
|
||
| // TODO: Check |
There was a problem hiding this comment.
that would be nice to not have TODO's on a brand new clean PR
|
@chrismatheson any update on that? As I mentioned, I'm more enclined to handle this at CI time instead of with the repository, as we don't commit build artifacts. This goes also for parse-server for example as the compiled code is not committed for example. |
|
@flovilmart Hi! yeah i agree that the best solution would be to generate the definitions at CI / publish time. However I'm not sure thats possible ? Maybe I'm missing something, but this codebase is using flow types? and i haven't seen any way to create a typescript definition from the flowtype system? |
|
How did you generate the definitions in the 1st place? |
|
@flovilmart i didn't, it was hand made by others and was part of the definitely typed project. the idea of merging into this project was to get the description of the types closer to the code (like you mentioned) and actually to have the updating of those same typedefs go hand in hand with updating the code. :) plus its a much nicer dev experience for those who are using types. |
|
Yeah but if you don't provide a way to keep them up to date, that's quite useless and again, I believe I've been clear enough on how I'd like those to be distributed. Also, the packages are bundled as dependencies, we need them as devDependencies. Closing as not fitting the requirements, feel free to open a new PR that addresses the concernes expressed above. |
Taking the work allready done here and merge it into the project as per sugestions of typescript project
This will mean users of typescript & parse will have one less step to do (manually pull in types) and
hopefully changes in the definitions themselves will stay closer to the actual thing.
tests for this have been added as a pretest npm script (if it compiles with no warnings/errors then "tests" are passed)