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

Upgrade bower dependencies #2601

Merged
merged 4 commits into from
Jul 3, 2017
Merged

Upgrade bower dependencies #2601

merged 4 commits into from
Jul 3, 2017

Conversation

gnestor
Copy link
Contributor

@gnestor gnestor commented Jun 23, 2017

Follow up from #1227

This upgrades xterm.js and codemirror dependencies in bower.json.

I also removed the codemirror patch code in favor of installing codemirror from a fork repo because it seems like it will be easier to maintain that way (and it’s slightly less of a hack). Using the patch code, it isn’t obvious to any contributor except myself which part of codemirror needs patched and this patching needs to happen every time that codemirror is upgraded. Using a fork repo, we can simply rebase the patch branch to the most recent release commit on codemirror and replay the patch commit over it. For efficiency’s sake. the fork is under my name, but I can transfer it to the jupyter org or any other contributor can fork codemirror themselves and perform the same patch on their own branch.

@gnestor gnestor added this to the 5.1 milestone Jun 23, 2017
@gnestor gnestor requested a review from takluyver June 23, 2017 15:19
@takluyver
Copy link
Member

Thanks @gnestor . Looks like there's an issue on CI with building the JS with your codemirror fork.

@takluyver
Copy link
Member

Also, have we re-tested for the issue with the latest Codemirror? It might have been fixed, in which case we could drop our patch.

@gnestor
Copy link
Contributor Author

gnestor commented Jun 23, 2017

I tested vanilla codemirror 5.27.2 and I can't reproduce any of those bugs from #1967. I tested in Safari and Chrome. Can you give it a try to double-check?

@takluyver
Copy link
Member

I'll have a go with it, but I think that bug only ever affected Safari, so I can't check for it. If we can get another Mac user to test and find it working, I'd say ship the vanilla version and cross our fingers.

@takluyver
Copy link
Member

I had a very brief play with it in Firefox, it seems to be working OK.

@gnestor
Copy link
Contributor Author

gnestor commented Jun 23, 2017

Ya, it looks good to me too. Merge at will 👍

@takluyver
Copy link
Member

I'd like to get one more safari user (or at least, someone with a Mac who can run Safari) to try to reproduce #1967 while we're still thinking about it before we merge, because it will be harder to find the necessary information if it's reported again in a few weeks.

@gnestor
Copy link
Contributor Author

gnestor commented Jun 23, 2017

Ok, @rgbkrk or @minrk, could you check this out and test for codemirror single-space bugs (#1967) in Safari?

@takluyver
Copy link
Member

OK, let's merge this and hope that the problem is gone.

@takluyver takluyver merged commit 869fe78 into jupyter:master Jul 3, 2017
@gnestor gnestor deleted the upgrade-deps branch July 3, 2017 17:34
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants