Skip to content

React 15, Meteor 1.3, updated and cleaned deps, updated karma and eslint fixs #140

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Mokto
Copy link

@Mokto Mokto commented May 6, 2016

No description provided.

@Mokto Mokto mentioned this pull request May 6, 2016
@jedwards1211
Copy link
Owner

Please remove eslint-config-airbnb and switch back to using eslint:recommended. eslint-config-airbnb treats humans as if they are robots whose job is to produce absolutely perfect code. That's computers' jobs, but the problem is a lot of the eslint-config-airbnb rules don't support automatic --fix, so it becomes a huge hassle to fight against those rules when contributing to open-source projects that use them.

@jedwards1211
Copy link
Owner

(I understand that some eslint-config-airbnb rules are not things that could ever be automatically fixed, but those are usually the more reasonable, safety-oriented ones; there are still a lot of other rules that could be automatically fixed, yet eslint doesn't support it yet)

@jedwards1211
Copy link
Owner

Just look at the number of stylistic rules that don't have an automatic fix yet could be fixed automatically. It's inhumane.

@Mokto
Copy link
Author

Mokto commented May 7, 2016

Ok, I will be doing this this sunday ;)

@jedwards1211
Copy link
Owner

Sorry, it's been a long week. Thanks for making this PR! I'd be happy to revert the eslint config myself.

@jedwards1211
Copy link
Owner

Also, [email protected] may still not support websocket proxying -- I will have to check. If not, I will merge it into my fork that does support proxying and use that in this PR.

@jedwards1211
Copy link
Owner

I just checked, [email protected] doesn't support websocket proxying -- they claim this won't be fixed until verson 2. I use forks of major projects in this app skeleton for good reason ;)

@jedwards1211
Copy link
Owner

I'm getting this error when I try to run it: meteor/react-packages#179

Not sure how it worked for you, but I don't think Meteor 1.3 with their React packages is ready for prime time (for example see here: meteor/react-packages/issues/152)

Also, we should probably figure out how to be importing meteor files instead of using global variables, before releasing official 1.3 support in this project.

@Mokto
Copy link
Author

Mokto commented May 8, 2016

For createContainer error, I created a package.json in meteor_core which includes react-addons-pure-render-mixin and react 15.0.2. Maybe we should automate install ?

@Mokto
Copy link
Author

Mokto commented May 8, 2016

When you say import meteor files, you mean something like import Meteor from 'meteor/meteor' ?

Please let me know if you need me to do anything ;)

@jedwards1211
Copy link
Owner

@Mokto that's not really an appropriate fix for createContainer because with React also getting built into the webpack bundle, the one from a package.json in meteor_core is a duplicate.

One can use webpack externals to load the one from Meteor, but really, ReactMeteorData needs to be something we can require into the webpack bundle...

@Mokto
Copy link
Author

Mokto commented May 9, 2016

You are absolutely right @jedwards1211. Importing createContainer does not work.

I have two related questions :

  1. How do you import Meteor/ReactMeteorData into webpack ?
  2. Sometimes when I reload my page (App.jsx), I randomly get this error : Uncaught ReferenceError: Meteor is not defined. Any idea why ?

@jedwards1211
Copy link
Owner

jedwards1211 commented May 10, 2016

  1. Might have to do with ReactDOM.render happening outside of a Meteor.startup() call...

As far as 1), I'm not sure about importing Meteor. But you could import ReactMeteorData, because https://github.com/meteor/react-packages has a package.json; I don't think it's on npm, but you can just npm install meteor/react-pacakges to install from GitHub.

@Mokto
Copy link
Author

Mokto commented May 13, 2016

  1. npm install meteor/react-pacakges does not work in my case and package.json does not refer to any ReactMeteorData. But thanks for the explanation ;)

  2. Weird. Because this is what I have in my main_client.js

Meteor.startup(() => {
  ReactDOM.render(<App />, document.getElementById('root'));
});

@jedwards1211
Copy link
Owner

jedwards1211 commented May 13, 2016

It doesn't install because they need to add a version number to their package.json: meteor/react-packages#191

It doesn't matter if package.json doesn't refer to ReactMeteorData; you can just import it like

import ReactMeteorData from 'react-packages/packages/react-meteor-data/ReactMeteorData'

(though I think they should rename it to meteor-react-packages meteor/react-packages#192)

@alexpricedev
Copy link

I'm really interested in this. I've followed a similar process and I'm stuck at trying to import { Meteor } from 'meteor/meteor'

@jedwards1211
Copy link
Owner

@alexpriceonline what happens when you do that?

@alexpricedev
Copy link

Error: Cannot resolve module 'meteor/meteor'

@jedwards1211
Copy link
Owner

jedwards1211 commented Jul 7, 2016

@alexpriceonline I figured it out. 'meteor/meteor' is not a true npm package -- it's not in node_modules. Instead Meteor's home-rolled module system handles that statement.

To get import { Meteor } from 'meteor/meteor', we will need to have a shim script outside the Webpack bundle that sticks those imports into global vars, so that Meteor is processing that file, and then Webpack externals so that the imports in the webpack bundle just read those global vars.

@jedwards1211
Copy link
Owner

jedwards1211 commented Jul 7, 2016

Though @Mokto do Meteor's global vars still exist in 1.3? It would seem like it since your code changes don't include any import { Meteor } from 'meteor/meteor'. Are you sure you actually got it to run with 1.3?

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