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

switch us over to React 16 #2884

Closed
wants to merge 4 commits into from
Closed

Conversation

rgbkrk
Copy link
Member

@rgbkrk rgbkrk commented Oct 1, 2017

This switches all our use of preact to React, which also allows us to pull React and ReactDOM in via bower and use as a UMD module.

Since all our supported browsers are modern, I've used an actual class here as well as arrow functions (where appropriate, to replace use of that = this).

With this in place, js code can now load react and react-dom explicitly:

require(['react', 'react-dom'], (React, ReactDOM) => {
  // ...
})

@rgbkrk rgbkrk force-pushed the switch-to-react branch 4 times, most recently from 3d3dca8 to 4c0e779 Compare October 1, 2017 16:34
@rgbkrk rgbkrk requested review from Carreau, minrk and mpacer October 1, 2017 16:34
@rgbkrk rgbkrk changed the title [WIP] switch us over to React 16 switch us over to React 16 Oct 1, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 1, 2017

Whoop, a little bit more for me to fix on the shortcut editor.

@rgbkrk rgbkrk changed the title switch us over to React 16 [WIP] switch us over to React 16 Oct 1, 2017
@rgbkrk rgbkrk changed the title [WIP] switch us over to React 16 switch us over to React 16 Oct 1, 2017
@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 1, 2017

Conversion complete!

@rgbkrk rgbkrk requested a review from captainsafia October 1, 2017 16:55
@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 1, 2017

I am pretty darn sure the test failures are from ye ole phantomjs setup, and that I'm using features that are not supported on the old JS engine we use for testing. Should I go down the rabbit hole of modernizing the test runner? 🤔

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 1, 2017

@Carreau
Copy link
Member

Carreau commented Oct 2, 2017

+1;

Sorry about the test-runner mess; if it's not too hard that would be great; otherwise just mark the test as knownfail/skip and open an issue to fix it later;

Thanks for doing this; it's not easy to do cleanup work. ❤️❤️❤️

@@ -41,6 +38,8 @@
bootstraptour: 'components/bootstrap-tour/build/js/bootstrap-tour.min',
'jquery-ui': 'components/jquery-ui/ui/minified/jquery-ui.min',
moment: 'components/moment/min/moment-with-locales',
react: 'components/react/react.production.min',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to optionally use the development version of react if the notebook is built using the developer tools instructions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered that too, didn't want to go too far down the rabbit hole in lieu of other yaks that wanted shaving.

@mpacer
Copy link
Member

mpacer commented Oct 2, 2017

I'm not 100% comfortable with my understanding of the testing setup as it is… but I'm happy to work on modernising it if someone can point me in the right direction.

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 2, 2017

Sorry about the test-runner mess; if it's not too hard that would be great; otherwise just mark the test as knownfail/skip and open an issue to fix it later;

I think even if I adapt the code that I changed in here, it's probably react itself that is failing the current setup because they use Map, Set, and requestAnimationFrame -- https://reactjs.org/blog/2017/09/26/react-v16.0.html#javascript-environment-requirements

Either I stick in yet another shim for phantom or I go down the rabbit hole of adapting the tests.

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 2, 2017

I'm not 100% comfortable with my understanding of the testing setup as it is… but I'm happy to work on modernising it if someone can point me in the right direction.

Yeah, happily. I'll get something started following on with what I've started tinkering with in #2891.

@gnestor
Copy link
Contributor

gnestor commented Oct 9, 2017

otherwise just mark the test as knownfail/skip and open an issue to fix it later;

@rgbkrk What do you think of doing that as a temporary means to shipping the VDOM renderer?

@jasongrout
Copy link
Member

Since all our supported browsers are modern, I've used an actual class here as well as arrow functions (where appropriate, to replace use of that = this).

Nice!!!

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 9, 2017

I... Think it would mean all the js tests would be disabled.

@gnestor
Copy link
Contributor

gnestor commented Oct 9, 2017

otherwise just mark the test as knownfail/skip and open an issue to fix it later;

@Carreau Would that effectively disable all of our JS tests?

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 9, 2017

Yes.

Hence why I want to overhaul the testing setup.

@gnestor
Copy link
Contributor

gnestor commented Oct 9, 2017

@rgbkrk Sorry, I meant to mention @Carreau to ask him to clarify because it sounds like what he's proposing is a quick albeit temporary fix.

@takluyver
Copy link
Member

What's the next move on this?

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 31, 2017

I've been a bit stuck, hoping to switch out the testing framework (#2891).

@Carreau
Copy link
Member

Carreau commented Oct 31, 2017

I've been a bit stuck, hoping to switch out the testing framework (#2891).

Just skip the test, we have users to test the UI in depth. :-)

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 31, 2017

Rebased just in case, I'll see if skipping tests is feasible (I... kind of think it's all of them...)

@rgbkrk
Copy link
Member Author

rgbkrk commented Oct 31, 2017

There's a lot more to either skip or rewrite.

@rgbkrk
Copy link
Member Author

rgbkrk commented Nov 7, 2017

Does it seem like we could include core-js into our page, particularly for phantom? https://github.com/zloirock/core-js

@takluyver
Copy link
Member

I've opened #3321 to start a switch towards Selenium - would that get around the issues you're having with the PhantomJS tests?

@gnestor
Copy link
Contributor

gnestor commented Feb 12, 2018

It sure sounds like it would. @rgbkrk?

@rgbkrk
Copy link
Member Author

rgbkrk commented Feb 12, 2018

Yeah, I think so! Selenium is pretty well up to date and well-backed.

@blink1073
Copy link
Contributor

Closing since we're using react 16 now. Cheers!

@blink1073 blink1073 closed this Jun 3, 2020
@rgbkrk
Copy link
Member Author

rgbkrk commented Jun 7, 2020

Wait what?! OMG that's great!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 24, 2021
@rgbkrk rgbkrk deleted the switch-to-react branch October 20, 2023 01:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants