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

React.createElement #168

Closed
ngasull opened this issue May 29, 2016 · 29 comments
Closed

React.createElement #168

ngasull opened this issue May 29, 2016 · 29 comments

Comments

@ngasull
Copy link
Contributor

ngasull commented May 29, 2016

Hi there! Question for @developit :

I'm considering to use preact because 3kb for the React API is just amazing. However, react's ecosystem has a lot more tools at the moment especially react-hot-loader and enzyme for testing, which are not compatible with preact or preact-compat.

In order to benefit from both of both worlds, I'm importing react and react-dom in the code, use original react in development, and use an alias on preact when really making the bundle. This way I realized that createElement was simply not in preact, thus making it not a drop-in replacement of react. I think this is really unfortunate, in my case, and as this could have encouraged people taking advantage of preact.

Don't you think exporting createElement as an alias of h would be good?

I considered the .babelrc jsx pragma solution but this is really inconvenient as I'd like to manage choosing react or preact from my webpack configs.

NB: if you have better options to suggest me in terms of enzyme/hot-loader tooling I'm definitely interested 😃

@developit
Copy link
Member

@ngasull - hi! preact-compat exports a nearly identical interface to React & ReactDOM (combined) in order to meet the use-cases you described. It exports not only createElement but the various other things React components s expect, like .Children.

Does that work for your setup?

@ngasull
Copy link
Contributor Author

ngasull commented May 29, 2016

@developit: acutally, my setup is about switching between react and preact without changing the code, and without preact-compat. Note that the latter doesn't work with react-hot-loader, which is required.

React.createElement is an essential function, I have some ways to solve the switching issue in my own case, but I think this could be a very common scenario among /p?react/ users. Wouldn't it be both really cheap and convenient to export it as an alias of h in preact?

@developit
Copy link
Member

Hmm - mind expanding on the react-hot-loader issue? This is the first I've heard about it, seems like an issue worth solving regardless :)

As for the exports, it would be somewhat cheap yes, though as if yet preact has never had to duplicate things in order to emulate react. That role has intentionally been delegated to preact-compat in order to keep Preact as focussed aand lightweight as possible.

Another option though, while I'm thinking on it, would be to use a Babel or Webpack environment configuration value to switch the JSX Pragma between React.createElement and h for prod/dev.

@developit
Copy link
Member

@ngasull By the way, for react-hot-loader, I actually just recommend using the Webpack interface directly, since it's very simple:

// src/index.js

import React from 'react';
import ReactDOM from 'react-dom';

function init() {
    let App = require('./components/app').default;
    ReactDOM.render(<App />, document.body);
}

// always render once:
init();

// add hot module replacement:
if (module.hot) module.hot.accept('./components/app', init);

Just the last line is how you set up HMR for a whole project. Updates for all other components are accepted because they are dependencies of App (or whatever your root is).

@ngasull
Copy link
Contributor Author

ngasull commented May 30, 2016

@developit when setting up react-hot-loader with preact, it just threw me away because it manually looks for module react/lib/ReactMount. Note that I don't expect preact to expose this :) that's why I'm trying to switch between react and preact.

As for your suggestion of configuring module.hot manually, I'd like my component tree to keep as much state as possible. I'll check if this method works well in my setup.

As for enzyme, I'll come back when I have more info on why it breaks.

Thanks for considering this issue!

@ngasull
Copy link
Contributor Author

ngasull commented May 30, 2016

@developit so I figured out some things wrong with enzyme using preact-compat:

  1. Enzyme expects react to export a version string. At the moment, it crashes on this.
  2. After stubbing 1., I faced a TypeError: Cannot call a class as a function. enzyme uses react-addons-test-utils and I think that preact-compat is not compatible with them.

Do you have any better suggestion for testing in preact? Enzyme enables shallow rendering which would be really great to have in preact as well.

@developit
Copy link
Member

developit commented May 30, 2016

@ngasull Think it'd be kosher to just export an empty string? That'd at least let Enzyme call .substring() on it. I'll try to get preact-compat working with it.

For my own testing, I use JSX assertions (via preact-jsx-chai) for testing render() and pure functions and shallow rendering. For cases where I need to observe the DOM and react to it, I just use Preact's normal render() method and inspect the DOM or the returned component (via ref).

// shallow (no dom)
expect(<Foo />).to.equal(<Bar from="foo" />);

// deep (no dom)
expect(<Foo />).to.deep.equal(<div class="bar">bar</div>);

// dom example
let c, scratch = document.createElement('div');
render(<Foo ref={c => foo=c } />, scratch);
expect(c.state).to.eql({ foo: 'bar' });
c.handleFooOrSomething('baz');
expect(c.state).to.eql({ foo: 'baz' });

@ngasull
Copy link
Contributor Author

ngasull commented May 31, 2016

@developit any string would work as a version. I used "preact" locally and enzyme just fell back on its default, thanks for considering this change.

I didn't know about preact-jsx-chai. Really interesting stuff, I'll give it a try. However I'd love to have partial attributes comparison in my tests!

@ctrlplusb
Copy link

In relation to this. I tried to set up HMR using native webpack 2 features - i.e. no react-hot-loader. However, I received the following error during the first HMR attempt:

TypeError: Cannot redefine property: type
    at Function.defineProperty (native)
    at :/~/preact-compat/dist/preact-compat.js:39:1
    at PropTypes (:/~/preact-compat/dist/preact-compat.js:2:1)
    at Object.module.exports.exports.__esModule (:/~/preact-compat/dist/preact-compat.js:5:2)

Here is the code in question:

Object.defineProperty(VNode.prototype, 'type', {
    get: function () {
        return this.nodeName;
    },
    set: function (v) {
        this.nodeName = v;
    }
});

Probs gets executed on the first run, therefore fails on second. Would a quick prop check suffice? I'm happy to PR if you feel so.

@developit
Copy link
Member

developit commented Jul 18, 2016

@ctrlplusb ah nice catch - yeah a check or even just throwing writable:true into the priority definition would be a reasonable fix.

PR would be awesome! Anything patching stuff into Preact should really be setting {writable:true,configurable:true}.

@ctrlplusb
Copy link

Sweet, on it. :-)

@ctrlplusb
Copy link

ctrlplusb commented Jul 19, 2016

I have created a simple example of how to use native HMR of webpack 2 with preact/preact-compat.

https://github.com/ctrlplusb/preact-compat-hmr

@nishtacular
Copy link

Hey guys, I'm trying to get my head around this, but not having much luck. I tried the following but keep getting the Error: Aborted because 123 is not accepted

Can someone point me in the right direction? Thanks

Command line:
cross-env NODE_ENV=development webpack-dev-server --hot --progress --colors --inline

function renderApp () {
  render(
    <Provider store={store}>
      <MainContainer />
    </Provider>,
    document.body
  ) 
}

renderApp()

if (module.hot) {
  module.hot.accept('./containers/MainContainer/MainContainer', renderApp)
}
[HMR] Cannot apply update. Need to do a full reload!
[HMR] Error: Aborted because 123 is not accepted
    at hotApply (http://localhost:8080/bundle.js:391:31)
    at hotUpdateDownloaded (http://localhost:8080/bundle.js:304:13)
    at hotAddUpdateChunk (http://localhost:8080/bundle.js:284:13)
    at webpackHotUpdateCallback (http://localhost:8080/bundle.js:5:12)
    at http://localhost:8080/0.73251b577e1a4a03f928.hot-update.js:1:1

@developit
Copy link
Member

developit commented Jul 19, 2016

@nishtacular it seems like you might have the wrong file path being passed to module.hot.accept.
Are you able to post the rest of that file?

@developit
Copy link
Member

Here's a thought (relating to the original question): what if preact provided a submodule for these types of super basic compatibility shims?

import preact from 'preact/compat';

preact; // the normal export, but decorated with low-hanging fruit compat fixes:

preact.createElement  // alias of h()

If we were to do this, are there any other trivially simple aliases we could add?

@inancgumus
Copy link

@ngasull Can you answer please cause we are following the discussion? :)

@ngasull
Copy link
Contributor Author

ngasull commented Aug 15, 2016

@developit I really like the idea of isolating "compat aliases" into a module! 😄

So far I didn't need anything else than createElement on top of preact's API. Would definitely love having this shim in an installable module 👍

@developit
Copy link
Member

Sweet. If anyone wants to do a PR I'm open to it, or I can try to get something out tonight.

@ngasull
Copy link
Contributor Author

ngasull commented Aug 15, 2016

@developit I'd be glad to do the PR 👋 just checking about the naming: importing from preact/compat looks confusing to me because so close from preact-compat. Shouldn't we have something more explicit like preact/compat-aliases?

@developit
Copy link
Member

Agreed, that name would be a bit chaotic. compat-aliases works, or even just preact/aliases. Trick will be whether we transpile, or just provide a commonjs entrypoint that does the mapping..

@ngasull
Copy link
Contributor Author

ngasull commented Aug 15, 2016

I'm just not sure on how to expose this submodule: today, "main" module targets preact in dist, so users count on transpiled dist/preact.js to be valid. If I provide an src/aliases.js that imports src/preact.js:

  • Am I sure that importing aliases via module bundlers will actually work the same as the transpiled version?
  • Shouldn't we expose this in a nicer way than preact/src/aliases? Should I create an aliases.js at the root of the project that imports src/aliases.js

@developit
Copy link
Member

might have to transpile to /. The file can be .gitignore'd, and then force-added via the "files" section in package.json. Only weird bit is that the paths would be actually have to be relative to /.

If we're cool with saying only ES Modules are supported for preact/aliases, then the aliases.js in the root setup in your second bullet seems the nicest to me.

@ngasull
Copy link
Contributor Author

ngasull commented Aug 15, 2016

One last suggestion with transpiling: preact-aliases.js and preact-aliases.min.js could simply be exposed in dist directly. React team seem fine exposing dist/react-with-addons.js. And importing preact/dist/preact-aliases would only be done in someconfiguration file anyway, right?

Adding files to the root just doesn't feel clean 😅

@developit
Copy link
Member

I agree that root files isn't the nicest, though it's pretty easy to transpile to the root and only have them deployed in the npm package. Are we over-complicating it by doing it in the preact module instead of just doing preact-compat/light or even its own module (eg preact-aliased)?

@ngasull
Copy link
Contributor Author

ngasull commented Aug 17, 2016

Ok for transpiling to the root then! I prefer having aliases in preact rather than preact-compat though, because aliased version don't actually contain more features than just preact.

If agreed, I'll take care of the PR!

@developit developit added this to the 5.8 milestone Aug 22, 2016
@developit
Copy link
Member

I think we can finally close this since import { createElement } from 'preact/aliases' is now officially a thing in 6.0.0 (beta)!

@windyGex
Copy link
Contributor

windyGex commented Jan 4, 2017

I want to replace my code from react to preact, but I got a problem that is my test case. I use alias of webpack for preact, like follow code

alias: {
'react':  require.resolve('preact-compat'),
'react-dom': require.resolve('preact-compat'),
}

Because I use react-addons-test-utils to simulate events, so this will be error, what is package can instead of react-addons-test-utils that could be not modify my code?

When I use enzyme, I got the same problem.
image

@developit
Copy link
Member

@windyGex there is an open issue in preact-compat for enzyme compatibility: preactjs/preact-compat#82
It's on hold for the moment because Enzyme itself is actually working on a much better solution:
enzymejs/enzyme#742

@windyGex
Copy link
Contributor

windyGex commented Jan 4, 2017

ok, thx, I will continue to focus on this.

marvinhagemeister added a commit that referenced this issue Mar 2, 2019
Fix vnode ordering issue when devtools are open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants