What is the process of adding new samples?
JLarky opened this issue · 18 comments
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 :)
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.
a side note, i have submitted a PR to update the guidelines for contributing in #80
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, @mhegazy and I think that the right way forward for samples is the following:
- Show code that works, not provide users with a skeleton.
- Samples should not have the kitchen sink. For instance, a sample shouldn't use a test framework unless the sample is specifically supposed to showcase using a testing framework.
- Every sample should have some kind of extensive documentation - whether this be part of the
README.mdor a quick-start page in our documentation. I think README and comments go a long way here.- One thing that I'll specifically mention here is that all build files should have documentation.
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.
For your sample, do you believe it should be a new one, or an augmentation to the existing react sample?
@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.
A sample is meant to showcase a specific technology or toolchain integration with TypeScript; it is not meant to be a template that users would use to get a project going.
@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.
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.
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 @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 :(
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:
@DanielRosenwasser I guess in this case I will wait for es6-babel-react-flux-karma to be fixed with requirements you just described.
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 😄
@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.
@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?
@DanielRosenwasser sure, no problem. I even signed CLA, thank's for updated contribution instructions that I can actually follow ;)
@DanielRosenwasser so I guess you done all of this in 468e0f1, contributors guide was fixes, so we can close this issue?
except that link in https://github.com/Microsoft/TypeScriptSamples/blob/master/README.md is broken now :)
Hey @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
Btw I suspect I would have ditched flux in favour of redux if I had my time again...
PS so surprised I didn't set noImplicitAny - that's so unlike me! 😄
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.
@DanielRosenwasser I would also like to see that 'let' is more used instead of 'var', as TypeScript recommends it.
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.