search

meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
573 stars 158 forks source link

POC of new system using direct npm import #171

Closed tmeasday closed 8 years ago

tmeasday commented 8 years ago

Hey @stubailo,

This is just a start but I thought I'd take some comment from you now (I'm guessing you are the most familiar with the initial React implementation). Is this what you imagine the new version of react-runtime would look like?

I've tested it out with the simple-todos-react app and it seems to work.

Some questions:

  1. What exactly is the difference btw react-runtime-dev and react-runtime-prod? And how do you achieve that difference using npm requires as opposed to browserify magic?
  2. How can I test out all the shim/sham business? Or do we think this is no longer required? cc @benjamn
    • alternatively is there a way to get the nice error message without using browserify or should we just not worry about it?
stubailo commented 8 years ago
  1. I think uglify and browserify have some magic that removes dead code inside if (process.env.NODE_ENV === 'production') { ... } blocks. React relies on this to include lots of extra debugging tools in development mode and strip them for production. We need to check if this still works in NPM. Or maybe they changed how they do it, I don't know. We actually have unit tests for this, so maybe we can run them?
tmeasday commented 8 years ago

Yeah I ran into a separate problem with running the tests -- basically that test-packages doesn't pickup the node_modules that installed in the app you are testing so you can't run it here.

Maybe it'd work if I (temporarily) put down a node_modules folder in the package itself. I'll try that.

tmeasday commented 8 years ago

Here's an issue on that topic: https://github.com/meteor/meteor/issues/6391

tmeasday commented 8 years ago

Ok, @stubailo, here's where I'm at with this.

  1. I can't make the tests run, see https://github.com/meteor/meteor/issues/6401
  2. However I have confirmed that it does the right thing in both modes (we now set process.env.NODE_ENV as expected both on client and server).
  3. This'll mean we just need a single react-runtime package, which is nice.
  4. It doesn't appear to strip out the unnecessary code paths however. I'm not sure if there's anything we can do about that. Do you think it's a big deal?
tmeasday commented 8 years ago

It doesn't appear to strip out the unnecessary code paths however. I'm not sure if there's anything we can do about that. Do you think it's a big deal?

Note that it's the same if you require React the "correct" way import React from 'react'; --- i.e. from the npm module directly --- in your app.

So perhaps this is just an small issue with our client-side bundling.

Perhaps I should open an issue about this.

tmeasday commented 8 years ago

Ok, I created an issue for that. I think it's fair to say it's a separate problem to this package. Will continue with this PR.

tmeasday commented 8 years ago

Ok, I think this is probably good to go, modolo https://github.com/meteor/meteor/issues/6401 and https://github.com/meteor/meteor/issues/6402 (neither of which I'm sure will be solved for 1.3)

stubailo commented 8 years ago

Looks great. So how do we want to manage the dependency on Meteor 1.3 beta?

tmeasday commented 8 years ago

Just the same as any non-core package I think -- release this as 0.14.4-1.beta.1 or something, then release 0.14.4-1 right after 1.3 comes out. (I'm not 100% sure what the version number should be TBH. Maybe this should be version 1.0?

tmeasday commented 8 years ago

Ok, I worked around the inability to run test packages by building an app that you can run app tests on. Apart from a problem with --production (meteor/meteor#6405), I feel good about this.

tmeasday commented 8 years ago

In the process I realised that I wasn't doing the React.addons thing.

Do you think I should

a) Force the user to npm install all the react addons, as I have done here

b) Just create React.addons for the ones that exist?

stubailo commented 8 years ago

Hmm if react.addons doesn't even exist anymore then we should just stop supporting it IMO.

tmeasday commented 8 years ago

Well I mean this package (react-runtime) is really just legacy at this point anyway, right? People should just import 'react' + co directly.

So I'm not sure that the fact that React.addons isn't a thing any more is a good reason to not keep it. IDK.

From the user's perspective we have three possible outcomes:

  1. [if we do a) from above] They run their app, upgrade this package, and are told Error: react@0.14.x not installed. (etc ... where etc includes all these packages). If they check the migration guide they'll realise they need to either stop using this package or npm install a bunch of stuff. Otherwise maybe they'll muddle through installing the correct packages [1].
  2. [if we do b) from above] They run their app, upgrade this package, see this error for react and react-dom, perhaps muddle through, and then start getting undefined symbol 'React.addons' randomly throughout their app. If they realise what's going on they can npm-install some more stuff to solve the problem.
  3. More or less the same as 2. except they have to change their code that uses React.addons to solve the problem.

I think 1. is the best outcome in terms of smooth upgradability. But maybe we should be pushing people harder away from this package?

[1] I think check-npm-versions should probably say something like:

Error: Required NPM packages not installed. Please `npm install` the following packages:
 - react-addons-transition-group@0.14.x not installed
 - ...

Please read the guide.meteor.com/using-packages.html#peer-depedencies for more information about this error.

(what do you think?)

stubailo commented 8 years ago

So the only problem here is that any package that depends on react-runtime will then cause you to need to install every single React addon in order to run your app at all. And then they would all get bundled for the client.

I think React 15.0 will save us from this problem since we can actually just bump the major version...

I dunno. Maybe we should print a warning saying "warning: you are using package X that depends on react-runtime. please contact the maintainer to stop using it"

tmeasday commented 8 years ago

So the only problem here is that any package that depends on react-runtime will then cause you to need to install every single React addon in order to run your app at all. And then they would all get bundled for the client.

Yeah. I mean this was happening before, you just didn't need to do it explicitly.

I think React 15.0 will save us from this problem since we can actually just bump the major version...

I think we just don't release a 15.0 version of this package, that's all. This package is purely here for package back-compat. People shouldn't be using it directly.

I dunno. Maybe we should print a warning saying "warning: you are using package X that depends on react-runtime. please contact the maintainer to stop using it"

It'd be nice to have a deprecation strategy :/ I think for now people will run into the missing imports problem, hopefully read the migration guide, avoid using the package directly and hopefully it'll filter through to package maintainers.