RFC: Switch from createElement to the default pragma

[PWAStudioBot]

From @DrewML on February 2, 2018 18:19

Edit: For those looking to contribute, the following changes need to be made:

1. Remove pragma here, and Babel will fall back to the default
2. Change createElement import in each file (ex: import { Component, createElement } from 'react';) to import React from 'react'; or import React, { Component } from 'react';
3. Submit a PR to Peregrine to make the same updates

---

Part of our reasoning for not using things like TS is that we didn't want to diverge too much from code people will see in examples.

Right now, we're over-writing Babel's pragma for React.createElement to just createElement. This is going to be surprising for people copy/pasting code from tutorials.

Proposing we go back to the default pragma.

Copied from original issue: magento-research/venia-pwa-concept#22

[PWAStudioBot]

From @zetlen on February 2, 2018 18:47

We overwrote the pragma because we thought we might have to monkeypatch createElement, right? It wasn't just so we could write import { Component, createElement } from "react"; instead of import React, { Component } from "react", right?

[PWAStudioBot]

From @DrewML on February 2, 2018 18:52

We overwrote the pragma because we thought we might have to monkeypatch createElement, right

No, that wouldn't have anything to do with it.

[PWAStudioBot]

From @DrewML on February 2, 2018 18:53

It wasn't just so we could write import { Component, createElement } from "react"; instead of import React, { Component } from "react", right?

I can't speak to that - I didn't set this up initially.

[PWAStudioBot]

From @jimbo on February 2, 2018 21:49

In the original concept, you (@zetlen) had indicated we might consider replacing React with another library, such as Preact. While I don't think Andrew or I ever supported that idea, it was on my mind when creating this repo that we might have to change every file with JSX at some point.

Whichever pragma we use has to be imported by every file containing JSX. Whether it's createElement or h, we have to import the function, but we never call it. Because the source doesn't use it, but the transpiled code does, it's sort of a hidden dependency. Lint rules for unused variables actually have to carve out an exception for your JSX pragma.

The problem with import React is that createElement is only one of the methods on the React object. If you remove the dependency on createElement by switching to a library with a different JSX pragma, such as h, you can't safely replace import React with import { h } because you don't know what other methods of React may be in use in that module. Explicitly importing createElement is less ambiguous, easier to lint, and easier to find and replace if necessary.

That was the reasoning behind it, anyway. If we're sticking with React for good, it doesn't make much difference what pragma we use. I think comparing this to TypeScript is apples to oranges; this is minor, TS is major.

[PWAStudioBot]

From @DrewML on February 13, 2018 16:28

you can't safely replace import React with import { h }

This would never be something we have to do, though. If Preact was used, we would have to use preact-compat, which already maps createElement and the other expected React exports

[PWAStudioBot]

From @DrewML on February 13, 2018 16:32

Can we make a fake movie poster for I, Robot but instead it's you, Zetlen

[zetlen]

I know this seems like a low priority, but the more code we make and the more contributors we have, the bigger and uglier a merge this will be downstream. I think we can say two things:

1. The most we are likely to diverge from React would be preact-compat, which would make it safe to import React by itself.
2. If in the future we want to do something else, it would be a significant enough change that a large merge would make sense. As it is now, this is a large potential merge with no appreciable change in functionality.
3. If I can make the final call and say that we go ahead and switch to import React from 'react', then this is a perfect "good first issue", and we need lots of those!

@jimbo @DrewML what say you?

[jimbo]

If someone outside our team wants to take it on, I'm all for it. 👍

[DrewML]

Yeah I'd like to see it done (by community, of course) asap. Full instructions are in the op so should (hopefully) be an easy fix for someone.

[pcvonz]

Hey, is there progress being made on this? If not I can give it a go.

[gavinPeterson98]

Yeah I haven't had a chance, go for it

…

On Tue, Jul 31, 2018, 2:36 PM Paul ***@***.***> wrote: Hey, is there progress being made on this? If not I can give it a go? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#21 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMLcEOC5zSIpURCwR2in8pAtNl64jzTuks5uMLHWgaJpZM4UnC_f> .

[pcvonz]

Hey, I've made all the changes here.

However, I had to edit the .eslintrc.js file in both peregrine and venia-concept:

https://github.com/pcvonz/pwa-studio/blob/default-pragma/packages/venia-concept/.eslintrc.js

It'd definitely be better to just edit @magento/eslint-config so I'll probably revert the changes to those files and submit a pull request there. Does that sound good?

[pcvonz]

Updated my pwa-studio fork to use my fork of @magento-eslint here

• Linted all files to search for any linting errors. Removed a CreateElement import from ContainerChild.js

[pcvonz]

Ok, I could use some help on this one. It seems that some of the tests fail because they can't find the createElement function. Here is an example of one of the failures:

 FAIL  packages/venia-concept/src/components/Gallery/__tests__/gallery.spec.js
  ● Test suite failed to run

    ReferenceError: createElement is not defined

      31 |                     regularPrice: shape({
      32 |                         amount: shape({
    > 33 |                             value: number.isRequired
         |                                                                         ^
      34 |                         }).isRequired
      35 |                     }).isRequired
      36 |                 }).isRequired

      at Object.<anonymous>.emptyData.map (src/components/Gallery/items.js:33:76)
          at Array.map (<anonymous>)
      at Object.<anonymous> (src/components/Gallery/items.js:33:32)
      at Object.<anonymous> (src/components/Gallery/gallery.js:17:40)
      at Object.<anonymous> (src/components/Gallery/__tests__/gallery.spec.js:13:16)

[zetlen]

@pcvonz Took me way too long to get to this one. Thanks for volunteering and getting your hands dirty, man.

I cloned your branch and discovered two things:

1. There was another place where the Babel pragma was hiding: packages/venia-concept/.babelrc. Jest was using that .babelrc when running the Venia tests, so it using the pragma and expecting createElement.
2. There was an old implementation of fromRenderProp in Venia which called createElement directly. That file is no longer necessary, since fromRenderProp moved to Peregrine in 7a8ea9a.

I put the fixes in a patch for you, so you can just apply it to your index with git apply instead of merging some tertiary pull request. I hope that makes sense to you--let me know if it doesn't.

0001-chore-Update-.babelrc-and-rm-redundant-files.patch.txt

[pcvonz]

Rad! Patch applied and merged with master. I'll submit a pull request shortly.