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

Upgrade 1.10.2 #2

Closed
wants to merge 2 commits into from
Closed

Upgrade 1.10.2 #2

wants to merge 2 commits into from

Conversation

jankapunkt
Copy link

@jankapunkt jankapunkt commented Jun 30, 2020

This is an attempt to upgrade the project to Meteor 1.10.2 and fix compatibility issues with deprecated packages.

TODO

  • upgrade release to 1.10.2

  • update npm packages

  • get scss running

  • fix sass deprecation warnings on startup

  • get server running without startup errors

  • update React.createClass with createReactClass

  • add custom package version for utilities:meteor-griddle

  • fix react-disqus-thread

  • get client running without startup errors

  • remove Blaze completely (incl. templating compiler, use static-html instead, may involve replacing packages with dependencies to Blaze as well)

  • ... what else?

@SachaG
Copy link
Member

SachaG commented Jun 30, 2020

Nice work! Feel free to also remove a lot of the now-unnecessary code, like handling access permissions, Gumroad purchases, user accounts, etc.

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

It looks like there have been a lot of React Breaking Changes from version 0.14.x to 16.3.1, but I'm hoping most of them won't impact us.

It's probably useful to review the React Change Log and search "Breaking Changes" on their readme page to help sort them out: https://github.com/facebook/react/blob/master/CHANGELOG.md

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

I got the app running on my desktop, and initially got past a bunch of npm error where packages were missing, so just installed those. I cleared all the errors on the server side.

So far there is one major React upgrade the deprecated React.createClass and replaced it with createReactClass from a new split off package on npm called create-react-class

Migration details here: https://reactjs.org/blog/2017/04/07/react-v15.5.0.html#migrating-from-reactcreateclass

I found 47 instances of React.createClass that need to be updated, so I'm troubleshooting that now.

I think overall this is going to be a very easy project to get up-to-date!

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

The project is also dependant on this package 📦 on atmosphere and it needs the same React.createClass update to createReactClass, this should be very easy!

See this file: https://github.com/meteor-utilities/Meteor-Griddle/blob/master/MeteorGriddle.jsx

@SachaG
Copy link
Member

SachaG commented Jul 1, 2020

I think that's only used for the admin area so you can probably drop the dependency altogether.

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

Okay, good to know 👍 I did submit an issue on the package, we could also very easily fork it an republish the package under a new name. Any chance you could nudge them to update that package @SachaG ?

I think only 2-3 lines of code need to be updated 😀

I opened the issue over here:
meteor-utilities/Meteor-Griddle#22 (comment)

@SachaG
Copy link
Member

SachaG commented Jul 1, 2020

@mullojo I know for a fact that the author of that package is a pretty uncooperative and lazy person. Even if I nudge them I doubt it will change anything. Knowing them, they'll probably waste their time watching woodworking YouTube videos instead of doing any productive work for once. So I wouldn't put too much hope into them updating it.

But you can drop the dependency like I said, I'm pretty sure the package is not used anywhere in the contents of the book.

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

Haha, okay, gotcha 😎 I commented out that package for now in the Meteor packages file # utilities:meteor-griddle ✅ which resolved that error

@jankapunkt
Copy link
Author

jankapunkt commented Jul 1, 2020

I know for a fact that the author of that package is a pretty uncooperative and lazy person. Even if I nudge them I doubt it will change anything. Knowing them, they'll probably waste their time watching woodworking YouTube videos instead of doing any productive work for once. So I wouldn't put too much hope into them updating it.

This is hilarious 😆

Ok besides that, I will implemented the aforementioned fixes and add commits. Once everything is working I will change this PR to be ready for review :-)

Edit: I updated the TODO list

@jankapunkt
Copy link
Author

@mullojo @SachaG I just checked the React docs, wouldn't it be better to create React components as defined in the docs? It may take more time to rewrite but as far as I can see is create-react-class just a wrapper for the deprecated non-ES6 React.createClass

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

You are correct on the "wrapped" approach, but I don't think it matters much to update to the latest recommended approach. I think there are other things in the project that are not the latest standards, but I don't see any risk in just getting it patched with less effort 😀 Thanks for updating the checklist!

@jankapunkt
Copy link
Author

If you have any updates to commit, I think you might be able to just add them to this PR. If not, I can add you to the fork repo as collaborator.

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

I think I had to make a PR from this PR, or at least that's all I could do with the permissions. You can see my changes in the new PR at least. Changes are committed. I started with a fork from this PR.

@jankapunkt
Copy link
Author

@SachaG could you create a development branch or an 1.10.2 branch or something? So we can split the PR's more effectively and keep the discussion within an issue maybe?

@mullojo
Copy link
Collaborator

mullojo commented Jul 1, 2020

Okay 👌 so I was able to clear 100% of the console errors on the client now with everything up to date.

I just committed my new changes to the other PR, #3

Also React.PropTypes was deprecated and needed to be changed to just PropTypes

And new imports were needed:

import PropTypes from 'prop-types';

React blog: https://reactjs.org/blog/2017/04/07/react-v15.5.0.html#migrating-from-reactproptypes

Also, for now I commented out everything to do with Griddle which is from the atmosphere packages that needs updating.

The app only loads a "Loading..." text right now, so I'll have to look at what the app is supposed to to tomorrow, I'm on Los Angeles time, so signing off.

Also, I have an empty database obviously, so I'm seeing some server console text showing me that the database is empty. Obviously there is nothing in the database to publish & subscribe to.

@SachaG maybe you can take a look if you have some data in a database somewhere to run it with 👍

@SachaG
Copy link
Member

SachaG commented Jul 1, 2020

@jankapunkt @mullojo I've made you both admins so you can manage forks and PR properly :)

@mullojo ideally it'd be good if this worked with an empty database, since this would let people clone and run the repo locally. By the way the contents of the book will be prefilled into a chapters collection on startup so that part at least should work out of the box.

@jankapunkt
Copy link
Author

Ok, I just opened a new branch 1.10.2 and will close this PR and reopen a local PR that allows us to work together.

@mullojo you can safely keep your PR and just change the base branch from master to 1.10.2. From there we can both add commits to your PR

@jankapunkt jankapunkt closed this Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants