treycordova / nativejsx

JSX to native DOM API transpilation. :yellow_heart: <div> ⟹ document.createElement('div')!
MIT License
154 stars 14 forks source link

Inline prototypes are added unconditionally to all modules #14

Open odzb opened 7 years ago

odzb commented 7 years ago

It would be nice if inline prototypes were added only to modules that actually need the prototypes. Currently they are added to any module that is being processed by nativejsx, even to modules that don't have any JSX at all.

treycordova commented 7 years ago

Hey, mind giving 64360206ec5850ae0ac847db57483a4b1c8a28e3 a test run when you have the chance? It has file-by-file detection now.

treycordova commented 7 years ago

If it does what you'd like, I'll slice a new release with it.

odzb commented 7 years ago

It now removes most, but not all, of unnecessary inline functions. It's not perfect, but it got better. I think it's worth a release, and then maybe someone can improve this further.

Thank you for your work! nativejsx is an awesome tool.

odzb commented 7 years ago

Apparently, the changes in 6436020 do it right. It's issue #15 that causes code modifications to appear in unwanted places, which also causes inline functions to be inserted there.

treycordova commented 7 years ago

Alright, I've removed <noscript>. I'll be cutting a release of 4 after I confirm everything is OK with the build libraries like gulp, etc.

treycordova commented 7 years ago

4.0.0 is out, but I'm not so sure we've quite nailed this one. If you have time to vet the changes, that would be much appreciated.

odzb commented 7 years ago

I can confirm that now I don't see these functions being added to modules where there is no JSX (at least in my codebase). But for it to be completely perfect, it would need to inline only those functions that are actually used in the module (e,g, if a particular module uses __setStyles, but doesn't use neither __setAttributes nor __appendChildren, then only __setStyles will be included).

Also I must say that I consider global prototypes a bad idea overall. At least not something to be enabled by default. If two modules on the page need different versions of the prototypes, or someone uses these method names for something else, bad things might happen. On my project I can totally see this happening. If you care about having to insert copies of the same functions in multiple places in the same Webpack bundle, you could put them in an NPM module that just exports the functions (instead of registering them in global prototypes). But inlines are also totally fine for me because they are small enough.

treycordova commented 7 years ago

There's a new setting to use module-based inclusion:

nativejsx.parse(jsx, {
  prototypes: 'module'
})

Which will require an import of necessary helpers in your JSX files:

import { setStyles } from 'nativejsx';

function template(styles) {
  return <div style={styles} />;
}
odzb commented 7 years ago

I still prefer to inline this stuff. Now if only we could stop the Webpack loader from adding global prototypes all the time: https://github.com/treycordova/nativejsx-loader/issues/5