This repository was archived by the owner on Oct 12, 2022. It is now read-only.
Repository navigation
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
What is the process of adding new samples? #79
Description
Recently I wrote blog post about TypeScript and I used one of the samples as base for it. I wonder is it possible now to get this back into upstream repo? After reading guidelines for contributing it almost seams like there's no way to push any code at all :)
Activity
mhegazy commentedon Mar 18, 2016
Thanks for the interest. the main aim is to make sure that samples are of high-quality and are maintained. For your sample, do you believe it should be a new one, or an augmentation to the existing react sample?
We would definitely be interested in a PR if you would be willing to contribute one.
mhegazy commentedon Mar 18, 2016
a side note, i have submitted a PR to update the guidelines for contributing in #80
DanielRosenwasser commentedon Mar 18, 2016
The issue I have is that the original sample has the problem of being a little overkill, but you seem to have benefited from it. Maybe you have an alternate viewpoint.
While #80 covers the bases, Mohamed Hegazy (@mhegazy) and I think that the right way forward for samples is the following:
README.mdor a quick-start page in our documentation. I think README and comments go a long way here.So given that I have a barebones React project that I'm planning to put out, and given that the old sample doesn't conform to these guidelines anyway, I think we should replace the old sample entirely with this in mind.
JLarky commentedon Mar 18, 2016
Mohamed Hegazy (@mhegazy) I'm personally fine either way, but I think that original sample was 1 to 1 correlation with javascript implementation which is valuable by itself and my sample is deviation that many of existing flux users are not going to appreciate, while many typescript users will :) That's why I did it as new sample instead of changing original one.
Daniel Rosenwasser (@DanielRosenwasser) I think that makes sense, but I choose original sample as base for exactly the opposite reason :) since it was complete example of something that people will use instead of being just a piece of the puzzle you would have to work on and on. But in long term having too many examples like that will result it exhausting all combinations of technologies and frameworks :) That's why I created this issue, since I didn't want to participate into entropy too much :) and wanted some guidance with making my work helpful, instead of a burden.
Does it mean Flux+React or just React? If it means Flux, I guess we can make it work this way: I can wait until you make this project and decide either I can update my code and/or article to use it, or I will just leave things as they are right now with code hosted in my fork, so Microsoft repository will continue to be clean and I will continue to have mine as is :)
Also, now I feel bad for John Reilly (@johnnyreilly) who was working on es6-babel-react-flux-karma since I feel responsible for bringing topic up, if it can cause his sample to be removed :(
DanielRosenwasser commentedon Mar 18, 2016
My sample just uses React.
It sounds like the best thing is to fix up the current example to make it canonical, recommended TypeScript. Things I'd like to see are:
--noImplicitAnyenabled.any, unless the definition of the type is so large that it's extremely inconvenient.README.mdtogulp,src, andtest.JLarky commentedon Mar 18, 2016
Daniel Rosenwasser (@DanielRosenwasser) I guess in this case I will wait for es6-babel-react-flux-karma to be fixed with requirements you just described.
DanielRosenwasser commentedon Mar 18, 2016
Our initial understanding was that you were planning to fix up or replace the original sample. Right now we're pretty bogged down, so we're not going to be able to take this on for a while. If you'd like to lend a hand, we'd appreciate it 😄
JLarky commentedon Mar 18, 2016
Daniel Rosenwasser (@DanielRosenwasser) that depends on whether original author of es6-babel-react-flux-karma is willing to change it with requirements you proposed and how much time I will have to be able to fix this :) my original goal was to get good react+flux base example that I won't be needing to modify much and add just the changes I described in the article. I didn't know that es6-babel-react-flux-karma wasn't complying to standards of this repository. That doesn't mean that I don't see value in fixing it, it just means I rather prefer original author to fix it.
DanielRosenwasser commentedon Mar 19, 2016
Yaroslav Lapin (@JLarky) how about this - why don't you send us out a PR and when I get the chance, I'll reconcile the two samples? Down the line, we can do a full-blown formatting fix. Does that sound reasonable?
JLarky commentedon Mar 19, 2016
Daniel Rosenwasser (@DanielRosenwasser) sure, no problem. I even signed CLA, thank's for updated contribution instructions that I can actually follow ;)
JLarky commentedon Apr 5, 2016
Daniel Rosenwasser (@DanielRosenwasser) so I guess you done all of this in 468e0f1, contributors guide was fixes, so we can close this issue?
JLarky commentedon Apr 5, 2016
except that link in https://github.com/Microsoft/TypeScriptSamples/blob/master/README.md is broken now :)
johnnyreilly commentedon Apr 5, 2016
Hey Yaroslav Lapin (@JLarky),
Don't feel too bad for me! Just in case you're interested the sample came to be after I worked on an es6/Babel project of pretty fair complexity. The project was a success but could definitely have been delivered faster with a type system. I ported that project to TypeScript when 1.6 shipped and it got JSX support. So the resulting code has pretty much 0 TypeScript idioms; just javascript+types. TBH the type system was of far more use in the stores; they were a world of map/reduce where types proved really helpful. This doesn't apply to the sample in which the store doesn't do much. The React components were pretty much dumb views (intentionally) and so the type system was a less significant gain.
So I see what you've wanted to add; good call. I'm glad this appears to have been resolved to everyone's satisfaction. I just want there to be useful samples out there! I didn't actually realise the changes that had been made until I saw this thread this morning.
All the best,
John
johnnyreilly commentedon Apr 5, 2016
Btw I suspect I would have ditched flux in favour of redux if I had my time again...
johnnyreilly commentedon Apr 5, 2016
PS so surprised I didn't set
noImplicitAny- that's so unlike me! 😄johnnyreilly commentedon Apr 5, 2016
Final PS (probably): the motivation for this sample was creating an example which allowed writing both es6/es2015 and JSX. TypeScript provides type safe compilation, babel provides transpilation to es5. The amended sample still satisfies both of these goals.
It's a shame the title doesn't reflect the es6 goal anymore but I haven't checked the other repos, there may be others doing exactly the same and so maybe it's not worth drawing attention to.
wstaelens commentedon Nov 25, 2016
Daniel Rosenwasser (@DanielRosenwasser) I would also like to see that 'let' is more used instead of 'var', as TypeScript recommends it.
johnnyreilly commentedon Nov 25, 2016
I do actually mean to update this sample to demonstrate using types acquired from npm in the build process. Not sure when I'll get to it but it is on my mind.