google / mathsteps

Step by step math solutions for everyone
https://socratic.org
Apache License 2.0
2.11k stars 276 forks source link

use typed javascript in mathsteps #87

Open evykassirer opened 7 years ago

evykassirer commented 7 years ago

options:

curious what you think @aelnaiem

aelnaiem commented 7 years ago

I love types and flow is a great tool, so I think it's a good idea. Longer-term goal makes sense.

hmaurer commented 7 years ago

You might also want to consider Typescript here. I have used both and they each have pros and cons but it seems Typescript would be more appropriate for this project. It has better editor integration and it is much more widespread, making it easier for new contributors to get started.

kevinbarabash commented 7 years ago

I'm cool with either.

evykassirer commented 7 years ago

edited the issue to reflect what we're talking about better

yeah @hmaurer that makes sense. I'm not super familiar with either, but if they both do a good job and typescript is more common that sounds like a good reason to use it :)

hmaurer commented 7 years ago

@evykassirer I think we can close this for now; it's probably not something that will be implemented in the near future (following month or so) and it might be clearer if open issues actually correlate to things being worked on. Maybe we could have a list on the wiki of "long term goals" and open issues for them once we actually start work on one of them?

hmaurer commented 7 years ago

Relevant: https://dev-blog.apollodata.com/javascript-code-quality-with-free-tools-9a6d80e29f2d#.mfe1gsx20

evykassirer commented 7 years ago

I think it's worth keeping open and tagging with 'long term goals' as it is right now, so that there can be discussion on it (like what you posted yesterday!)

If people want to find things to work on, I hope they can filter out things with the long term label on them. Does that seem reasonable?

Thanks for looking through these and helping clean things up :) I'm also a fan of checking things off and having fewer things on my todo list and having things clean!

nitin42 commented 7 years ago

Should I try Flow ? and see how it goes ?

evykassirer commented 7 years ago

I think we were considering TypeScript instead. We should probably make an educated decision before going forward with one. If you can implement typed javascript in some files but not others, you could try it out for a file or two to see what it would look like :)

kevinbarabash commented 7 years ago

We should probably hold off until https://github.com/socraticorg/mathsteps/pull/144 lands. Sorry for dragging my feet with that. Definitely encourage experimentation though. :)

nitin42 commented 7 years ago

This may help https://github.com/Microsoft/TypeScript-Handbook

elu00 commented 7 years ago

Hi all, Since TS is a superset of JS, shouldn't migrating to TS simply integrating the TS compiler? According to this documentation page, all migration involves is literally just changing the file extension, since static types are actually optional. EDIT: If it's ok with the other maintainers, I can try to send in a PR for the migration to TS within the next couple days; Resharper seems to have a really good tool to assist in the migration.

evykassirer commented 7 years ago

thanks to @elu00 for this great progress:

https://github.com/elu00/mathsteps-ts

aliang8 commented 7 years ago

What I gather through a little bit of research:

Background (hehe idk):

Porting:

What it does and general notes:

TS:

Flow

Notes:

Both have this:

Pro Flow:

Against Flow:


nitin42 commented 7 years ago

Bad error messaging is due to poorly configured .flowconfig. I've been through this. For example -

[ignore]
.*/node_modules/.*
.*/index.js

[include]
.*/node_modules/styled-components/.*
[libs]

[options]

In the above example I've included the dependency styled-components in the [include] section because I am also type checking the styled components in my library. So I need to tell Flow that include this dependency also (else it would give daunting errors). Here the author of styled-components has added support for the Flow but this is not the same for the other node modules. So yes I totally agree on the bad error messaging and also that mathjs don't have the support for Flow, so it may be beneficial if we use Typescript.

nitin42 commented 7 years ago

One more thing, as @kevinbarabash is working on the new parser, I was thinking that it may help (me or someone else) to include the support for Flow in mathsteps. So If a developer is using mathsteps directly or we expose the api for the new parser in future, we should definitely include support for both Flow and Typescript. ?

kevinbarabash commented 7 years ago

The parser I'm working will have a different AST structure. As part of that work I'm adding a rule rewriting system. This will allows changes to be defined as strings of math with placeholders, e.g.

const rule = defineRule("#a + 0", "#a");
if (canApplyRule(rule, oldNode) {
    const newNode = applyRule(rule, oldNode);
}

You can follow that work at https://github.com/kevinbarabash/math-parser/issues/27.

I think that having a system like this will simplify the existing search/transform functions substantially, but it will mean lots of changes to those files. If people are interested in adopting a system like this, then I think it's probably best to start using Flow/Typescript in files outside of search.

evykassirer commented 7 years ago

opinion from Kevin on gitter:

I think for library work, TS is the right call. I think the stricter nature of structural typing will catch more issues.

evykassirer commented 7 years ago

it seems like it would be a good idea to wait for Kevin to make more progress until we start implementing typed javascript (adding blocked to the tags) - though we can keep talking about it, and if someone thinks there'd be benefit in adding it now I'm definitely open to talk about it

there are some other issues open for bugs and functionality that seem like a better place to put focus on for now (and I'll be adding a bunch more in the next few days!)