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

Include latest version of jQuery in tutorial #3498

Merged
merged 1 commit into from
Mar 25, 2015

Conversation

AnSavvides
Copy link
Contributor

The commit message should be descriptive enough :)

I have already submitted a CLA in the past.

@zpao
Copy link
Member

zpao commented Mar 24, 2015

We should perhaps even update this further and match what we do in the html we ship in the tutorial repo: https://github.com/reactjs/react-tutorial/blob/master/public/index.html

Let me know if you want to do that or we can always to that later. It hasn't been an issue either way.

@AnSavvides
Copy link
Contributor Author

That's a good point!

Looking at the tutorial repo I can see it uses jQuery 2.x, which doesn't support IE8, whereas React does if you use some polyfills right? Would it make sense to use jQuery 1.x both here and in the tutorial for those that might be seeking to develop against IE8?

Let me know what you think and I'll update this branch accordingly and open a PR in the react-tutorial repo if need be :)

@zpao
Copy link
Member

zpao commented Mar 25, 2015

I don't think it's super important to make the tutorial compatible with IE8. IE8 is usually an afterthought and if you do need to support it, you probably have some experience and know what you would need to do differently (including using an appropriate version of jQuery and not just copy and pasting code from tutorials).

@iamdustan
Copy link
Contributor

any thoughts of removing jquery for fetch (potentially with a reference to https://github.com/github/fetch) since it’s only present for $.ajax?

@AnSavvides
Copy link
Contributor Author

Ooh that's a good idea actually @iamdustan :) What do you think @zpao?

@zpao
Copy link
Member

zpao commented Mar 25, 2015

I like the idea but I'm a bit torn that it might be too new. While $.ajax has it's downsides, it's relatively familiar. Even if you've never used it, it's pretty simple to understand. fetch on the other hand uses Promises which have another step to actually understand.

We don't do it in the tutorial but elsewhere we talk about aborting xhr during componentWillUnmount. This is trivial with native xhr & $.ajax but is actually currently impossible with window.fetch.

So for now, let's just stick with what we have. We may want to revamp the tutorial entirely in the future and we can consider it then.

@iamdustan
Copy link
Contributor

fwiw, $.ajax also returns promise-like objects. My main reasoning is to avoid referencing jQuery and React at all, to hopefully prevent just one person from putting it in their unwittingly and then depending upon it instead of the model React provides.

Great point concerning aborting. Seems like a big oversight to the fetch design.

@zpao
Copy link
Member

zpao commented Mar 25, 2015

True, I forgot about jQuery's deferreds. And that's a good point. I'm not opposed longer term, but let's keep this change simple. I love the enthusiasm though - let's chat more about what else we can do to make some broader changes to the tutorial.

Include latest version of jQuery 2.x in tutorial
@AnSavvides
Copy link
Contributor Author

Yeah not being able to abort is a bit of a deal breaker - that's a very good point. It looks like there is an issue for adding support for abort - maybe sometime in the future window.fetch will be a viable solution.

Cool, so I've updated to jQuery 2.x so this should be ready to go :) Thanks for the nice conversation @zpao and @iamdustan 👍

zpao added a commit that referenced this pull request Mar 25, 2015
Include latest version of jQuery in tutorial
@zpao zpao merged commit 029a526 into facebook:master Mar 25, 2015
@zpao
Copy link
Member

zpao commented Mar 25, 2015

👍 thanks!

@AnSavvides AnSavvides deleted the patch-1 branch March 26, 2015 07:28
zpao added a commit that referenced this pull request Apr 3, 2015
Include latest version of jQuery in tutorial
@zpao zpao mentioned this pull request Oct 1, 2015
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