Address getting extension working with updated jupyter lab #64

playermanny2 · 2019-03-31T23:05:07Z

This commit gets the extension working with latest Jupyterlab

Changes include:
package updates in package.json,
syntax error fixes in index.ts and voyagerpanel.js,
filetype fixes for opening .csv and other supported filetypes correctly in index.ts,
changes for prop and react error fixes caused by updating of packages in tsconfig.json

known bugs so far:
export/save doesn't work,
some .csv may not open saying invalid format
columns with quotes (Ex: "date") cannot be parsed correctly

… package updates, syntax error fixes, filetype fixes for opening .csv correctly, and tsconfig changes for prop and react error fixes

playermanny2 · 2019-03-31T23:05:51Z

Ooops looks like i need to clean up the commented code, but before can you please review and let me know if that line is needed or not

…nted code in voyagerpanel.ts

…dated yarn package

… With inside menu item div

playermanny2 · 2019-04-12T15:43:12Z

Hey @saulshanabrook having a bit of trouble getting the cypress test to work...not too sure what I'm missing tbh. this is my first time working with cypress, but it looks pretty straightforward, it seems as if the mousemove is command is not triggering a visible menu when hovering over 'Open With'...seems odd as it fires the event and adds the new .p-Menu div onto the DOM.

I'm not sure if this is an actual issue with cypress, but it works fine when i manually hover -- I posted details about the issue on cypress... any ideas on what could be happening here, kind of stumped? :/

saulshanabrook · 2019-04-12T15:49:02Z

@playermanny2 Thanks for looking into the testing.

I will take a look at this locally.

playermanny2 · 2019-04-12T16:59:32Z

@saulshanabrook appears to an issue with our environment specifically... was able to do some testing in cypress-io/cypress#3945 with one of the contributors...Not sure if i messed something up internally within jupyterlab so now it can't properly receive those programmatic hover commands.

Down the rabbit hole...haha

jredding

My primary comments are around whitespace modifications, which can make it a little difficult to ascertain what has changed between code commits. Ideally if those could be rolled back it would make future reviews easier.

Can we review the If/Else if within voyagerpanel.ts is there a better way to handle this? It seems as though adding "Parse: Auto" would be fine in the else clause or is that not the case?

src/voyagerpanel.ts

jredding · 2019-04-14T22:44:34Z

package.json

  },
  "devDependencies": {
    "@types/node": "^9.6.5",
-    "typescript": "~2.4.1",
+    "typescript": "~3.4.1",


On all these version numbers is there a method to make them a little less precise?
Ex. ^3.4.1 vs ~3.4.1

I do not have enough knowledge up the upstream systems to understand if the minor/subminors would contain breaking changes. Looking to make this more resilient to modifications to dependencies.

we should be able to make this ^, I also can't confirm if further down the line this would cause breaking changes, but I think it'll probably be a better chance to take, as it would probably be a full new version(like 4.x) rather than the 3.x so we should be safe for a while.

jredding · 2019-04-14T22:46:12Z

src/index.ts

+              wdg.id = filename+(temp_widget_counter++);
+              wdg.title.closable = true;
+              wdg.title.iconClass = VOYAGER_ICON;
+              const tracker = new InstanceTracker<VoyagerPanel_DF>({ namespace: 'VoyagerPanel_DataFrame' });


a lot of these changes are whitespace modifications - can those be reverted? Would make it a little easier to identify the modifications to the code.

i'll look into reverting the changes...appears there's a way to stage only whitespace changes...It's most likely stems from windows linespace ending vs unix endings

playermanny2 · 2019-04-20T01:20:49Z

@saulshanabrook @jredding

Dug into a bit more, was able to isolate the cypress testing issue to Jupyterlab version ^0.35.

Open With -> Voyager appears correctly on 0.34.12 -- only thing i haven't been able to get working right is that after opening a test file in voyager and reloading, the voyager menu is not there (via cypress), however it works normally if i click without cypress and reload. I suspect this could be another weird bug with this jupyterlab version.

Overall, I think we have two options moving forward, let me know what you think is best

Revert core @jupyterlab/ dependencies to be on ^18.0, and change travis jupyterlab version to 0.34.12 for testing.This also means jupyterlab intalls must only be ~0.34.x in order to stay within the 18.0>19.0 dependency
Only change travis jupyterlab version to 0.34.12 for testing purposes. This means extension can work with only the latest juyterlabs (^.35.x)

I'm leaning a bit towards 2, I get it's a bit wonky to test under a different version, but I think the more we keep the extension on previous versions of jupyterlab, the worse it will be. Not to mention the testing on 0.34.12 will only be temporarily until we can hopefully get this issue fixed.

Not sure why this would be an issue with the new Jupyterlab ^.35.0 but we can create a post in the repository to start conversations about how to get that fixed.

@jredding not 100% sure how to revert only the whitespace changes, would it be okay to leave for now? in the future definitely can focus on creating cleaner diffs

jredding · 2019-04-23T21:23:18Z

I'm going to leave the whitespace decision up to @saulshanabrook - personally I think it would be good to revert those. I know it's a pain, but it does make it much cleaner to understand what changes were made so when this needs to be updated again we have a good historical reference.

In regards to the tests.. did the HTML structure change between .34 and .35?

The Cypress tests is stating:

CypressError: Timed out retrying: Expected to find element: '.p-Menu-item div:contains("Open With")', but never found it.

In .35.4 I see this in the final html

<li class="p-Menu-item p-mod-active" data-type="submenu">
   <div class="p-Menu-itemIcon"></div>
    <div class="p-Menu-itemLabel">Open With</div>
    <div class="p-Menu-itemShortcut"></div>
    <div class="p-Menu-itemSubmenuIcon"></div>
</li>

So could the test be written to look for 'div.p-Menu-itemLabel:contains("Open With")', ? (here)

or do you think this is a loading issue (i.e. looking for it prior to it being rendered?)

playermanny2 · 2019-04-24T16:53:56Z

@jredding the DOM didn't change for those menu items between versions. You can either reference using a more general .p-Menu-item with cy.contains to search the children, or you can access it using .p-Menu-itemLabel specifically.

This issue appears to be a bug rather than a change in DOM structure. Happy to set-up some time in order to show the issue in more detail

saulshanabrook · 2019-04-24T19:03:55Z

@playermanny2 I can replicate your issues locally.

Here is the changelog between 34 and 35, I will look for relevant changes. Thanks for helping track this down.

saulshanabrook · 2019-04-24T19:06:54Z

This looks like it could be the relevant change: jupyterlab/jupyterlab@ec0a573#diff-457faa59a5ec6fc44e00a122e66d5abc

saulshanabrook · 2019-04-24T19:16:57Z

@playermanny2 I couldn't figure out how to get the tests to pass.

I am fine merging this in as is, I tried it locally and it seemed to work.

saulshanabrook · 2019-04-24T19:18:13Z

Only change travis jupyterlab version to 0.34.12 for testing purposes. This means extension can work with only the latest juyterlabs (^.35.x)

I would just say, forget the tests for now. If you can do this easily, that's fine, but the version are usually pinned and so it might be more trouble than its worth.

I am happy with the state it's in, feel free to merge it in as you see fit.

playermanny2 · 2019-04-24T23:03:18Z

@saulshanabrook Awesome, yea I'll go ahead and merge this in as is for now so we can have a working repo...once merged, will you be able to take care of updating the npm package?

As far as the updates that caused the test to not work, do you think that it's a bug within cypress or jupyter? or is it something that just requires updating to use the right API and we should address in another ticket?

saulshanabrook · 2019-04-24T23:12:56Z

@saulshanabrook Awesome, yea I'll go ahead and merge this in as is for now so we can have a working repo...once merged, will you be able to take care of updating the npm package?

Sure, I will publish a new version.

As far as the updates that caused the test to not work, do you think that it's a bug within cypress or jupyter? or is it something that just requires updating to use the right API and we should address in another ticket?

I really don't know... I think it is probably a weirdness with how JuptyerLab is doing even handling. You could open up an issue on the main repo, with steps to reproduce, if you are motivated.

Thanks again for pushing through on this PR! It's great to have this working again :D

playermanny2 · 2019-04-25T01:43:44Z

@saulshanabrook I'll open up an issue on the main repo with these details.

My pleasure on the PR, glad to be helping! Let's see what we can tackle next xD

Address getting extension working with updated jupyter which includes…

38bc023

… package updates, syntax error fixes, filetype fixes for opening .csv correctly, and tsconfig changes for prop and react error fixes

playermanny2 changed the title ~~Address getting extension working with updated jupyter which includes…~~ Address getting extension working with updated jupyter lab Mar 31, 2019

playermanny2 mentioned this pull request Apr 7, 2019

Extension is not working for jupyter lab 0.33.11 #56

Open

Cato and others added 3 commits April 11, 2019 00:04

Address updating travis build, dockerfile, and removing uneeded/comme…

58f7391

…nted code in voyagerpanel.ts

Address anaconda-platform to conda-forge source channel switch for up…

97b20f0

…dated yarn package

Address modfiying cypress opening_files test to look for correct Open…

dbd2c5f

… With inside menu item div

saulshanabrook mentioned this pull request Apr 11, 2019

Cannot install the Jupyter Lab extension #62

Closed

jredding reviewed Apr 14, 2019

View reviewed changes

Update cypress

6b0d970

playermanny2 merged commit e392987 into altair-viz:master Apr 24, 2019

playermanny2 deleted the get-build-working branch April 24, 2019 23:05

playermanny2 mentioned this pull request Apr 25, 2019

0.35.x Jupyterlab Handling Hover Events Unstable jupyterlab/jupyterlab#6242

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address getting extension working with updated jupyter lab #64

Address getting extension working with updated jupyter lab #64

playermanny2 commented Mar 31, 2019 •

edited

Loading

playermanny2 commented Mar 31, 2019

playermanny2 commented Apr 12, 2019 •

edited

Loading

saulshanabrook commented Apr 12, 2019

playermanny2 commented Apr 12, 2019

jredding left a comment

jredding Apr 14, 2019

playermanny2 Apr 20, 2019

jredding Apr 14, 2019

playermanny2 Apr 20, 2019

playermanny2 commented Apr 20, 2019 •

edited

Loading

jredding commented Apr 23, 2019 •

edited

Loading

playermanny2 commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

playermanny2 commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

playermanny2 commented Apr 25, 2019

Address getting extension working with updated jupyter lab #64

Address getting extension working with updated jupyter lab #64

Conversation

playermanny2 commented Mar 31, 2019 • edited Loading

playermanny2 commented Mar 31, 2019

playermanny2 commented Apr 12, 2019 • edited Loading

saulshanabrook commented Apr 12, 2019

playermanny2 commented Apr 12, 2019

jredding left a comment

Choose a reason for hiding this comment

jredding Apr 14, 2019

Choose a reason for hiding this comment

playermanny2 Apr 20, 2019

Choose a reason for hiding this comment

jredding Apr 14, 2019

Choose a reason for hiding this comment

playermanny2 Apr 20, 2019

Choose a reason for hiding this comment

playermanny2 commented Apr 20, 2019 • edited Loading

jredding commented Apr 23, 2019 • edited Loading

playermanny2 commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

playermanny2 commented Apr 24, 2019

saulshanabrook commented Apr 24, 2019

playermanny2 commented Apr 25, 2019

playermanny2 commented Mar 31, 2019 •

edited

Loading

playermanny2 commented Apr 12, 2019 •

edited

Loading

playermanny2 commented Apr 20, 2019 •

edited

Loading

jredding commented Apr 23, 2019 •

edited

Loading