feat: allow mode to be defined, default to fast

[andru]

Fix for #54.

This introduces a .mode() method to the API to allow the mode to be defined. Mode is set by Surface.componentWillMount if the mode has not been set beforehand. Defaults to fast.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[andru]

Hm. The tests are ok but the travis container is using an old version of node and choking on const.

I had to update some deps to get the tests running on my machine (old Jest wouldn't work on new node!)

/home/travis/build/reactjs/react-art/node_modules/jest-cli/bin/jest.js:12
const fs = require('fs');
^^^^^
SyntaxError: Use of const in strict mode.

[zpao]

Can you pull out the dependency changes into a separate PR (then you can also update the node version to 4 in .travis.yml)

[andru]

@zpao not quite sure how to proceed here. I can't get the tests running for any combination of node and the existing jest (see blank Travis output, I get the same on my machine, below). This led to me updating jest and, accordingly, the test suite.

With an updated jest dependency on a recent node it's fine, but without that dependency update in this PR it's failing silently. Any suggestions on how best to package this up?

npm test
npm info it worked if it ends with ok
npm info using [email protected]
npm info using [email protected]
npm info pretest [email protected]
npm info test [email protected]

> [email protected] test /Volumes/Files/Code/GitHub/react-art
> jest

npm info posttest [email protected]
npm info ok

[zpao]

How about this… are you ok with me syncing out some changes we have internally which overlap considerably and getting jest running so you can focus on the actual code changes you need to make for your PR? You's just strip the top 2 commits (git reset --hard HEAD~2), rebase on our master branch, and have a working travis ci and local env.

[sophiebits]

By the way: require() with a dynamic argument doesn't work in many bundlers, including Facebook's. We also don't want to send down code that we won't ever run. Perhaps we could support ReactART.setMode(require('art/modes/svg')) though.

[andru]

are you ok with me syncing out some changes we have internally which overlap considerably and getting jest running so you can focus on the actual code changes you need to make for your PR?

@zpao That's ok with me.

@spicyj I was wondering about whether that would be problematic. The first API I tried was requiring a mode file within the react-art package... require('react-art/mode/svg'). Subsequent require('react-art') calls would use the current mode. I dropped that method because it introduced a lot of new files, but maybe it's a better option?
setMode(require('art/modes/svg')) would be ok with me too, but it does expose internal dependencies which might be better abstracted away?

Since I submitted this PR there's also another method I was playing with: each Surface element has it's own renderer.. As an API, I think it's the most react-y, and it allows for an edge case I had where I wanted to render a primary component instance with Canvas and an auxillary instance with SVG.

e.g.

<Surface mode="svg">

</Surface>
<Surface mode="canvas">

</Surface>

It would require a restructure of ART to provide a factory for render instances, each with an isolated mode. In principle it's not a huge change but co-ordinating and agreeing on changes to both libraries might be some work. Is there some crossover between the contributors? Does this latter API seem useful?

[sebmarkbage]

I'm not a fan of dynamic dependency injection. I think it would be better to do this at the level of something like webpack with an override.

ART already works with different modes. Just not with shapes but we reimplement those here anyway. So it is plausible that we could change render mode per subtree. For example, using context to pass down the mode.

However, nobody has yet said WHY they want this. What's the use case? The primary feature of ART is to lock it down so that it doesn't matter which render mode it is. Is it because of browser bugs rendering to canvas? If so, could we maintain a UA blacklist that switches mode automatically for flaky browsers?

I was kind of hoping of going all in on Canvas in the future - like Fabric.js did.