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

Update version of babel-plugin-transform-incremental-dom #3

Open
mairatma opened this issue Dec 16, 2016 · 6 comments
Open

Update version of babel-plugin-transform-incremental-dom #3

mairatma opened this issue Dec 16, 2016 · 6 comments
Labels

Comments

@mairatma
Copy link
Contributor

babel-plugin-incremental-dom has been renamed to babel-plugin-transform-incremental-dom, and it has released a version with a fix that we need. We should update this preset to use this latest version, and with the new plugin name.

@mthadley
Copy link

I took a look at their Changelog and wanted to summarize the things we will need to address:

jsxDOMWrapper is now an Object

These used to be functions, however they were changed to Objects with the intention of improving performance. I know we at least have some code in IncerementalDOMRenderer that will fail now since it will attempt to call these objects as functions. We can probably handle these cases with something like this:

(fn.func ? fn.func : fn)(this.getRenderer().config_);

Keys

key attributes are now required to be the first attribute passed to an element. This is enforced by the plugin and babel will throw an error for anything like this:

<div class="test" key={i}></div>

The reason for this can be found here in the commit message. Since the use case seems a little strange, we should see if it is possible to disable this error through configuration. If not, or if we decide that this is a good practice that we should follow, it means breaking changes for probably most metal-jsx users.

prefix replaced with moduleSource

Before we could pass the prefix option something like IncrementalDOM and calls would be compiled like this:

IncrementalDOM.elementOpen('div');

However it has been removed and replaced with moduleSource, which instead produces something like this (more or less):

/* given moduleSource = 'incremental-dom' */
var _incrementalDOM = require("incremental-dom");

_incrementalDOM.elementOpen('div');

If the moduleSource option is not set, then the plugin will just assume that plain functions like elementOpen will be available and in scope.

Whether or not this option is set, it won't work with our current code. Right now we just have a import 'incremental-dom'; in IncrementalDomRenderer, which made them available via a IncrementalDOM global on the window.

Possible solutions are to either ask if they can add the prefix option back, or make sure there is a module that exports the necessary functions, with this preset's moduleSource pointing to it.

runTime replaced with runtimeModuleSrouce

This change is similar to moduleSource, but it instead affects iDOMHelpers. Given that adding this is only a performance optimization, it is not as high of a priority. Either way, the solution will probably be similar to how we handle moduleSource.

@mthadley
Copy link

Opened an issue about the key error here.

@pragmaticivan
Copy link

I still get warnings for that package been renamed. Are there still plans to come back to that topic? Thanks

@mthadley
Copy link

mthadley commented Nov 15, 2017

I think if someone has time, they should definitely look in to upgrading. From memory, there were also some performance improvements to the babel-plugin-transform-incremental-dom module, which I think is something we could benefit from.

/cc @jbalsas @Robert-Frampton

@jbalsas
Copy link

jbalsas commented Nov 17, 2017

We're scheduling this for our next batch of work!

@robframpton
Copy link

If we were to use their moduleSource option the way it's intended, I think it would result in some rather significant changes for metal-incremental-dom and any other packages that depend on the global IncrementalDOM variable.

I think we should talk with the maintainers about bringing back both the prefix and runTime options, as it would be much safer with less chances of introducing regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants