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

Require jinja2 2.10+ to fix extra escaping #7055

Merged
merged 8 commits into from Aug 21, 2019

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 20, 2019

References

Fixes #7053.

Code changes

Adds a requirement on jinja2 2.10+ to pick up pallets/jinja#718

User-facing changes

Fixes inability to load JupyterLab due to ill-formatted page config.

Backwards-incompatible changes

None

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Aug 20, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073 blink1073 added this to the 1.0.x milestone Aug 20, 2019
@blink1073 blink1073 changed the title require jinja2 to pick up pallets/jinja#718 Require jinja2 to pick up pallets/jinja#718 Aug 20, 2019
@blink1073 blink1073 changed the title Require jinja2 to pick up pallets/jinja#718 Require jinja2 2.10+ to fix extra escaping Aug 20, 2019
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

Fixes #6941.

That was reported with jlab 1.0.4, so I think that may be a separate issue.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Thanks, I just updated the top level description.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Note to self: conda-forge recipe will need an update as well.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

And jupyterlab_server, since it uses the same tojson filter.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

PR coming...

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

Add missing whatwg-fetch dependency

Huh. Why did this not complain before? This PR should not have changed this requirement, I think?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Other recent PRs were failing for the same reason. I think it may have come in transitively before.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Still failing. whatwg-fetch itself hasn't been updated in over a year.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

webpack-cli was updated 3 days ago...

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

% yarn why whatwg-fetch                                                                 outputconsole
yarn why v1.15.2
[1/4] 🤔  Why do we have the module "whatwg-fetch"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "whatwg-fetch@3.0.0"
info Has been hoisted to "whatwg-fetch"
info Reasons this module exists
   - "workspace-aggregator-3bc6e7ea-abd1-4618-aaa4-5dc09a06097c" depends on it
   - Hoisted from "_project_#@jupyterlab#example-app#whatwg-fetch"
   - Hoisted from "_project_#@jupyterlab#example-notebook#whatwg-fetch"
   - Hoisted from "_project_#@jupyterlab#example-terminal#whatwg-fetch"
   - Hoisted from "_project_#@jupyterlab#example-cell#whatwg-fetch"
   - Hoisted from "_project_#@jupyterlab#example-filebrowser#whatwg-fetch"
   - Hoisted from "_project_#@jupyterlab#example-console#whatwg-fetch"
   - Hoisted from "_project_#fbjs#isomorphic-fetch#whatwg-fetch"
info Disk size without dependencies: "64KB"
info Disk size with unique dependencies: "64KB"
info Disk size with transitive dependencies: "64KB"
info Number of shared dependencies: 0
=> Found "webpack-visualizer-plugin#whatwg-fetch@0.9.0"
info Reasons this module exists
   - "_project_#@jupyterlab#application-top#webpack-visualizer-plugin#react#fbjs" depends on it
   - Hoisted from "_project_#@jupyterlab#application-top#webpack-visualizer-plugin#react#fbjs#whatwg-fetch"
info Disk size without dependencies: "24KB"
info Disk size with unique dependencies: "24KB"
info Disk size with transitive dependencies: "24KB"
info Number of shared dependencies: 0
✨  Done in 1.58s.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

The webpack visualizer is new, right? It seems to be getting a very old version of the library, 0.9.0, from about 4 years ago.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Add a new resolution 😄?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Actually, webpack visualizer is unmaintained, we should be using https://github.com/webpack-contrib/webpack-bundle-analyzer.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

The webpack visualizer is new, right?

It was recently re-enabled: #6706

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

@vidartf, the failing test is test_build_custom_minimal_core_config. Do you mind taking a look at this one?

@vidartf
Copy link
Member

@vidartf vidartf commented Aug 20, 2019

@blink1073 The coreconfig test uses the package.json in staging. Should the changes in dev_mode/package.json be propagated? How does the other test builds know to use the dev mode package.json ?

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Aha! The whatwg-fetch requirement must come from one of the requirements that wasn't included in the minimal test.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

Python tests are passing now, thanks @vidartf!

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 20, 2019

@jasongrout, it is getting to be the end of my time at the keyboard for today. I can cut a patch release first thing tomorrow.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 20, 2019

Sounds good. I'm afk myself.

@blink1073 blink1073 merged commit 4b2cf55 into jupyterlab:master Aug 21, 2019
9 checks passed
@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 21, 2019

@meeseeksdev backport to 1.0.x

@meeseeksdev
Copy link

@meeseeksdev meeseeksdev bot commented Aug 21, 2019

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

  1. Checkout backport branch and update it.
$ git checkout 1.0.x
$ git pull
  1. Cherry pick the first parent branch of the this PR on top of the older branch:
$ git cherry-pick -m1 4b2cf55c184643950d270c359e8acb208bccdd15
  1. You will likely have some merge/cherry-pick conflict here, fix them and commit:
$ git commit -am 'Backport PR #7055: Require jinja2 2.10+ to fix extra escaping'
  1. Push to a named branch :
git push YOURFORK 1.0.x:auto-backport-of-pr-7055-on-1.0.x
  1. Create a PR against branch 1.0.x, I would have named this PR:

"Backport PR #7055 on branch 1.0.x"

And apply the correct labels and milestones.

Congratulation you did some good work ! Hopefully your backport PR will be tested by the continuous integration and merged soon!

If these instruction are inaccurate, feel free to suggest an improvement.

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Aug 21, 2019

I manually made the jinja2 dependency change and released 1.0.7.

@jasongrout jasongrout removed this from the 1.0.x milestone Aug 23, 2019
@jasongrout jasongrout added this to the 1.1 milestone Aug 23, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Sep 22, 2019
@blink1073 blink1073 deleted the require-jinja2 branch Mar 29, 2020
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.

3 participants