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

Phosphor upgrade #1762

Merged
merged 53 commits into from Feb 23, 2017
Merged

Phosphor upgrade #1762

merged 53 commits into from Feb 23, 2017

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Feb 21, 2017

Upgrade notes:

  • Most of the path changeovers were made using a variant of:
find . -name "*.ts" -exec sed -i '' -e 's/phosphor\/lib\/core\/token/@phosphor\/application/g' {} +
  • The bulk of the manual work was in conforming to the new Signal and ReadonlyArray APIs. Both resulted in cleaner code and APIs in JupyterLab.
  • Added common/vector.ts for now to preserve ObservableVector.
  • No changes to Phosphor were required for this PR.
@blink1073 blink1073 force-pushed the phosphor-upgrade branch from 5f4de1b to 39ccbc9 Feb 21, 2017
@@ -406,7 +406,7 @@ class CompleterWidget extends Widget {
let items = this.node.querySelectorAll(`.${ITEM_CLASS}`);
let subset = Private.commonSubset(Private.itemValues(items));
let query = this.model.query;
if (subset && subset !== query && subset.indexOf(query) === 0) {
if (subset && subset !== query && subset.ArrayExt.firstIndexOf(query) === 0) {
Copy link
Contributor

@jasongrout jasongrout Feb 21, 2017

ArrayExt.firstIndexOf(subset, query)?

Copy link
Contributor

@jasongrout jasongrout Feb 21, 2017

or just revert the change

@@ -271,7 +263,7 @@ class BreadCrumbs extends Widget {
if (error.xhr) {
error.message = `${error.xhr.status}: error.statusText`;
}
if (error.message.indexOf('409') !== -1) {
if (error.message.ArrayExt.firstIndexOf('409') !== -1) {
Copy link
Contributor

@jasongrout jasongrout Feb 21, 2017

Another .indexOf that should probably have been left alone.

Copy link
Contributor

@jasongrout jasongrout Feb 21, 2017

(There are a few more here, if you search for .indexOf, or in the new files, search for .ArrayExt.firstIndexOf)

Copy link
Member Author

@blink1073 blink1073 Feb 21, 2017

Yeah, the compiler is catching these for us. Whoops.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 21, 2017

Awesome, thanks! Searching the Changed Files tab for .indexOf shows a few places where there probably should not have been a substitution.

afshin
afshin approved these changes Feb 23, 2017
Copy link
Member

@afshin afshin left a comment

Whoo boy.

👍

@afshin afshin merged commit 4564240 into jupyterlab:master Feb 23, 2017
2 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Feb 23, 2017

nice, that was fast! I was dreading this huge (line number) change :).

@blink1073 blink1073 mentioned this pull request Feb 23, 2017
@blink1073 blink1073 deleted the phosphor-upgrade branch Feb 23, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants