search

altair-viz / jupyterlab_voyager

JupyterLab extension visualize data with Voyager
BSD 3-Clause "New" or "Revised" License
298 stars 35 forks source link

Address getting extension working with updated jupyter lab #64

Closed playermanny2 closed 5 years ago

playermanny2 commented 5 years ago

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

jupyterlab_voyager_working
playermanny2 commented 5 years ago

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

playermanny2 commented 5 years ago

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 commented 5 years ago

@playermanny2 Thanks for looking into the testing.

I will take a look at this locally.

playermanny2 commented 5 years ago

@saulshanabrook appears to an issue with our environment specifically... was able to do some testing in https://github.com/cypress-io/cypress/issues/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

playermanny2 commented 5 years ago

@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

  1. 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
  2. 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

working_cypress_jupyterlab_voyager working_cypress_jupyterlab_voyager_2
jredding commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

This looks like it could be the relevant change: https://github.com/jupyterlab/jupyterlab/commit/ec0a5737722b59118edb3dea36a4c877a959b9e0#diff-457faa59a5ec6fc44e00a122e66d5abc

saulshanabrook commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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