Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed #155

lneves12 · 2022-10-03T10:59:44Z

Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed.

Relying on the existing file custom.js has some limitation because it gets loaded after the main.js script is executed. That makes it hard to extend some behaviors like injecting security headers:

example:

      $.ajaxSetup({
        beforeSend: function (xhr) {
          xhr.setRequestHeader(
            "Authorization",
            `Bearer ${connectionDetails.password}`
          );
        },
      });

Here is a demo video demonstrating the new behavior:

fix.mp4

echarles · 2022-10-04T20:36:33Z

@lneves12 Thx for opening this. The first thought while reviewing this is about the name you have chosen custom which is a bit too generic I believe. Apart from that, I am trying to understand the issue you are addressing and how you solved it could you expland a bit more. I get your point around loading order, but would need more background and details if possible. Thx again!

lneves12 · 2022-10-04T21:23:43Z

Thanks for the quick reply @echarles

I am not sure I understood the custom name comment. I don't think we have the control to change that, because that's the name of the module registered on page.html:

     // error-catching custom.js shim.
      define("custom", function (require, exports, module) {
          try {
              var custom = require('custom/custom');
              console.debug('loaded custom.js');
              return custom;
          } catch (e) {
              console.error("error loading custom.js", e);
              return {};
          }
      })

I am not an expert in require.js though. Did you mean to change the define name to something else ?

To fix this I kinda got inspired on this old PR: https://github.com/jupyter/notebook/pull/1732/files
That PR fixes the same issue on jupter V4, but it seems it never got backported to master.

More about the use case:

In order to use the iframe we need to extend some of the behavior. One of the examples (there are more thing) is to inject a couple of security headers. The problem is that the current custom.js is executed after notebook.load_notebook(common_options.notebook_path);.

Therefore doing things like:

      $.ajaxSetup({
        beforeSend: function (xhr) {
          xhr.setRequestHeader(
            "Authorization",
            `Bearer ${connectionDetails.password}`
          );
        },
      });

will not work, because not all the requests will have the injected token.

lneves12 · 2022-10-04T21:46:50Z

Regarding the Linux JS Tests job failure, I am not sure yet what's going on, not sure if it's related.

All the tests are failing with Message: ReferenceError: Can't find variable: $ so it seems unrelated.

I am trying to run the tests locally, but not luck yet.

Even though I can run python3 -m nbclassic --port 8989

python3 -m nbclassic.jstest notebook is failing with /usr/bin/python3: No module named nbclassic

echarles · 2022-10-05T14:22:03Z

I am not sure I understood the custom name comment. I don't think we have the control to change that, because that's the name of the module registered on page.html:

You are right, thx for pointing to that content in the html page.

In order to use the iframe we need to extend some of the behavior. One of the examples (there are more thing) is to inject a couple of security headers. The problem is that the current custom.js is executed after notebook.load_notebook(common_options.notebook_path);.

Could it be that other cases do need custom.js to be run after the notebook loading?

Regarding the Linux JS Tests job failure, I am not sure yet what's going on, not sure if it's related. All the tests are failing with Message: ReferenceError: Can't find variable: $ so it seems unrelated.

The CI is not 100% green, but no test is failing with that error. I would say that it is related to these changes.

python3 -m nbclassic.jstest notebook is failing with /usr/bin/python3: No module named nbclassic

You can follow the needed steps to run the tests in

lneves12 · 2022-10-05T15:33:34Z

Could it be that other cases do need custom.js to be run after the notebook loading?

I would say no, in my opinion if there are cases like that they are probably badly written, because they can write code depending on jupyter triggered events (load, killed, etc).

btw this is how custom.js was being loaded in jupyter V4, it was just never backported to master. At least that's what i understood by reading the git history. 4.4.1 tag code: https://github.com/jupyter/notebook/blob/4.4.1/notebook/static/notebook/js/main.js#L27

The CI is not 100% green, but no test is failing with that error. I would say that it is related to these changes.
You can follow the needed steps to run the tests in

thanks, I will try again to make them run locally

echarles · 2022-10-05T17:14:39Z

I would say no, in my opinion if there are cases like that they are probably badly written, because they can write code depending on jupyter triggered events (load, killed, etc).

We have just discussed that during the community call and we came to a conclusion that it would be preferable to not merge such change to avoid the risk to break any existing extensions.

btw this is how custom.js was being loaded in jupyter V4, it was just never backported to master. At least that's what i understood by reading the git history. 4.4.1 tag code: https://github.com/jupyter/notebook/blob/4.4.1/notebook/static/notebook/js/main.js#L27

... we did not have that information while discussing during the community meeting. mmh would it be possible to hide that feature behind a flag on the command line. This will complexify quite a lot your PR, but it may be the safest way to move forward. WDYT?

lneves12 · 2022-10-06T13:35:55Z

I really doubt that loading custom.js before would be a big deal. It will just make the custom.js load more consistently. If something breaks, even if rare, it should be easy to fix. It doesn't seem like a breaking change, at least to me. If your custom code depends on the events triggered by the core, nothing should break. Also this change was already in the code in previous versions.

Anyway, I understand the concern. What do you think of having 2 files ? custom.js and custom-preload.js ? Introducing a feature flag seems to be quite complex. I would probably need to inject extra information in every handler from a quick look.

I will start implementing that solution, but let me know what you think and if that approach is reasonable.

Thanks!

echarles · 2022-10-06T13:38:15Z

Anyway, I understand the concern.

Thank you for your understanding.

What do you think of having 2 files ? custom.js and custom-preload.js ? Introducing a feature flag seems to be quite complex. I would probably need to inject extra information in every handler from a quick look.
I will start implementing that solution, but let me know what you think and if that approach is reasonable.

That sounds to me like a brilliant idea. Looking forward your PR.

lneves12 · 2022-10-06T14:22:37Z

@echarles done! just updated the PR with the new file solution. I also updated the description with a new demo video showing the new file behavior.

Can you rerun the CI tests again? I don't seem to have permissions to do that.

echarles · 2022-10-06T14:52:40Z

Kicked the CI job and this one is failing https://github.com/jupyter/nbclassic/actions/runs/3197888375/jobs/5222141888

lneves12 · 2022-10-07T14:26:28Z

Those seem to be failing as well in other PR: https://github.com/jupyter/nbclassic/actions/runs/3181760673/jobs/5186881765

There are 2 tests failing though in Selenium Tests only in Macos. I am still investigating why and how they can be related with my changes.

Could they be flaky ? Or my change made them being more flaky?

I will keep investigating, but any insight or help would be very welcoming! I am still very new to this project

Thanks!

echarles · 2022-10-07T17:25:07Z

Could they be flaky ? Or my change made them being more flaky?

Oh, maybe I have not looked correctly. Indeed, you just need to ensure that there are no regressions. We will work to make the CI 100% green, but for now we have some small failures that we know of. So if you could first double check that no regression is introduced, I can review.

lneves12 · 2022-10-07T17:51:36Z

@echarles could you try to rerun the tests? I rebased my branch.

I already reviewed a lot of code and the test and I can't understand why the test test_save_as_notebook.py is failing only in macbook. A retry after my rebase would confirm if this is really a regression or just flakiness.

echarles · 2022-10-07T19:49:25Z

@echarles could you try to rerun the tests? I rebased my branch.

Done

echarles · 2022-10-10T07:43:48Z

Some tests were failing, so I have rerun the failing ones. They still show issues which I think were not there before, like on https://github.com/jupyter/nbclassic/actions/runs/3206644310/jobs/5260741971, there is

Run python -m nbclassic.jstest notebook

Test group: notebook
Test file: /home/runner/work/nbclassic/nbclassic/nbclassic/tests/notebook/attachments.js
Timeout for http://localhost:8888/a@b/
Is the notebook server running?
FAIL ".CodeMirror-code" still did not exist in [1](https://github.com/jupyter/nbclassic/actions/runs/3206644310/jobs/5260741971#step:9:1)0000ms

lneves12 · 2022-10-10T09:23:42Z

@echarles I think I see the same errors happening in last master commit.

jstests -> https://github.com/jupyter/nbclassic/actions/runs/3207336103/jobs/5242111041

selenium -> https://github.com/jupyter/nbclassic/actions/runs/3207336100/jobs/5242110926

=========================== short test summary info ============================
FAILED nbclassic/tests/selenium/test_save_as_notebook.py::test_save_notebook_as
FAILED nbclassic/tests/selenium/test_save_readonly_as.py::test_save_readonly_notebook_as
============ 2 failed, 29 passed, 821 warnings in 312.27s (0:05:12) ============

let me know if I overlooked something

echarles · 2022-10-10T09:29:56Z

That's true. I'd like first to investigate what is wrong with the regression in main branch. How urgent is this for you?

lneves12 · 2022-10-10T14:05:38Z

@echarles it's a blocker for us, so it has some priority for us. However I guess you will not release a new version until the tests are green anyway.

I would be surprise if this change breaks any test.

How can I help ? I can try to look at the js tests in master, but I haven't been able to run them locally yet, so it's a bit annoying to fix them. I will try again.

The selenium tests seem to be failing only in macos, not sure if I will be able to replicate them. (I have linux)

echarles · 2022-10-10T16:14:10Z

I am investigating #156 which is I think the cause of the test regression.

lneves12 · 2022-10-11T10:52:54Z

thanks! @echarles

let me know if there is anything I can do to help

I don't mind helping fixing some of the tests and make the main branch green!

echarles · 2022-10-11T12:49:08Z

#159 on its way to fix the test in main branch. Once merged, you can rebase your PR.

PS: Sometimes it will be completely green, sometiimes you may have a few failures like 525 passed, 2 failed.

echarles · 2022-10-11T13:58:34Z

@lneves12 #159 is merged in master. Can you please rebase this PR?

…re the main.js script is executed

lneves12 · 2022-10-11T14:10:10Z

@echarles nice!

My branch is rebased on top of latest master. You can retry the tests

echarles

LGTM Thx @lneves12

echarles · 2022-10-11T14:35:04Z

I have merged. Are you looking for a new release?

lneves12 · 2022-10-11T15:04:48Z

@echarles thanks for all the help and quick replies!

yeah, that would be very helpful.

One question: it seems only the notebook version > 6.5.X relies on nbclassic. Do you know if there is an official release of 6.5 happening soon? or should we build our own docker images depending on the new release of nbclassic using the 6.5 branch? I am still new in the jupyter notebooks ecosystem, so any hint would be very helpful :)

We have been using the official docker hub images: https://hub.docker.com/u/jupyter

echarles · 2022-10-11T15:55:08Z

yeah, that would be very helpful.

Will cut a new nbclassic tomorrow.

Do you know if there is an official release of 6.5 happening soon?

@ericsnekbytes and @RRosio are preparing that. They can give you more details on the plan.

We have been using the official docker hub image

The docker image release is not managed in this repo. I will add this question to the agenda of the next notebook community meeting.

echarles · 2022-10-12T08:34:13Z

@lneves12 nbclassic 0.4.6 is just released with these changes.

jtpio mentioned this pull request Oct 6, 2022

Notebook Weekly Meetings Nov 2021 - Dec 2022 jupyter/notebook-team-compass#5

Closed

lneves12 force-pushed the fix_custom_load_order branch from 4e0d3c2 to ee63b51 Compare October 6, 2022 14:15

lneves12 changed the title ~~Fix custom.js load order to be before main.js~~ Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed Oct 6, 2022

lneves12 force-pushed the fix_custom_load_order branch from ee63b51 to 8c9aebf Compare October 7, 2022 17:47

echarles added the feature label Oct 10, 2022

adds new file custom-preload that is loaded in a different order befo…

85c68b1

…re the main.js script is executed

lneves12 force-pushed the fix_custom_load_order branch from 8c9aebf to 85c68b1 Compare October 11, 2022 14:09

echarles approved these changes Oct 11, 2022

View reviewed changes

echarles merged commit 327c9a0 into jupyter:main Oct 11, 2022

This was referenced Oct 14, 2022

custom-preload does not work for Notebook 6.5.x #162

Closed

Ensure custom preload is correctly handled jupyter/notebook#6580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed #155

Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed #155

lneves12 commented Oct 3, 2022 •

edited

Loading

echarles commented Oct 4, 2022

lneves12 commented Oct 4, 2022 •

edited

Loading

lneves12 commented Oct 4, 2022 •

edited

Loading

echarles commented Oct 5, 2022

lneves12 commented Oct 5, 2022 •

edited

Loading

echarles commented Oct 5, 2022

lneves12 commented Oct 6, 2022

echarles commented Oct 6, 2022

lneves12 commented Oct 6, 2022 •

edited

Loading

echarles commented Oct 6, 2022

lneves12 commented Oct 7, 2022

echarles commented Oct 7, 2022

lneves12 commented Oct 7, 2022

echarles commented Oct 7, 2022

echarles commented Oct 10, 2022

lneves12 commented Oct 10, 2022

echarles commented Oct 10, 2022

lneves12 commented Oct 10, 2022 •

edited

Loading

echarles commented Oct 10, 2022

lneves12 commented Oct 11, 2022

echarles commented Oct 11, 2022

echarles commented Oct 11, 2022

lneves12 commented Oct 11, 2022

echarles left a comment

echarles commented Oct 11, 2022

lneves12 commented Oct 11, 2022

echarles commented Oct 11, 2022

echarles commented Oct 12, 2022

Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed #155

Adds new file custom-preload that is loaded in a different order than custom.js before the main.js script is executed #155

Conversation

lneves12 commented Oct 3, 2022 • edited Loading

echarles commented Oct 4, 2022

lneves12 commented Oct 4, 2022 • edited Loading

lneves12 commented Oct 4, 2022 • edited Loading

echarles commented Oct 5, 2022

lneves12 commented Oct 5, 2022 • edited Loading

echarles commented Oct 5, 2022

lneves12 commented Oct 6, 2022

echarles commented Oct 6, 2022

lneves12 commented Oct 6, 2022 • edited Loading

echarles commented Oct 6, 2022

lneves12 commented Oct 7, 2022

echarles commented Oct 7, 2022

lneves12 commented Oct 7, 2022

echarles commented Oct 7, 2022

echarles commented Oct 10, 2022

lneves12 commented Oct 10, 2022

echarles commented Oct 10, 2022

lneves12 commented Oct 10, 2022 • edited Loading

echarles commented Oct 10, 2022

lneves12 commented Oct 11, 2022

echarles commented Oct 11, 2022

echarles commented Oct 11, 2022

lneves12 commented Oct 11, 2022

echarles left a comment

Choose a reason for hiding this comment

echarles commented Oct 11, 2022

lneves12 commented Oct 11, 2022

echarles commented Oct 11, 2022

echarles commented Oct 12, 2022

lneves12 commented Oct 3, 2022 •

edited

Loading

lneves12 commented Oct 4, 2022 •

edited

Loading

lneves12 commented Oct 4, 2022 •

edited

Loading

lneves12 commented Oct 5, 2022 •

edited

Loading

lneves12 commented Oct 6, 2022 •

edited

Loading

lneves12 commented Oct 10, 2022 •

edited

Loading