synacor / preact-i18n

Simple localization for Preact.
BSD 3-Clause "New" or "Revised" License
206 stars 18 forks source link

No display names, and function names are mangled. #3

Closed gnarf closed 6 years ago

gnarf commented 7 years ago

When rendering the <Text> component for instance, the name of the actual method in the dist output is g so you get <g id="id">default text</g> instead of <Text id="id">default text</Text> when you render this.

Adding Text.displayName = 'Text'; fixes this, and you may want to also add display names to the other components and theWithText HOC that is output. You can also update the uglify preferences to not mangle function names so console.log(require('preact-i18n').Text) would have the right "name" for the function in the first place.

This becomes more important if we have tools like enzyme or preact-render-spy doing something like this:

const context = shallow(<MyComponent />);
expect(context.find('Text').attr('id')).toEqual('mytranslation.id');

without displayName we need to search for g (which is subject to change due to uglification)

billneff79 commented 7 years ago

@gnarf Instead of using displayName, which is more of a react idiom and not one that preact really uses, have you tried converting the arrow function declaration of the components to actual function declarations to see if that fixes your issue? For example, instead of:

export const Text = ({ id, children, plural, fields }, { intl }) => { do: export function Text({ id, children, plural, fields }, { intl }) {

at times that style helps tools preserve the name better.

As for the build artifact, in order to have the smallest build we are going to continue to uglify the names of the functions.

If your tool is depending on every component to have discoverable name across every library, I think you are going to be fighting a losing battle. Perhaps instead of relying on component names in your selectors, you could use function signatures for components. i.e. instead of:

expect(context.find('Text').attr('id')).toEqual('mytranslation.id'); //Text is a string do expect(context.find(Text).attr('id')).toEqual('mytranslation.id'); //Text is a function reference

Because this is true...

expect((<Foo/>).nodeName).to.equal(Foo) //true for class components and functional components

...you might be able to modify https://github.com/mzgoddard/preact-render-spy/blob/master/src/sel-to-where.js to add another case like:

const selToWhere = sel => {
  if(typeof sel === 'function') { 
     return {nodeName: sel}; //check against compiled component signature
  }
  else if (/^\./.test(sel)) {
    return {attributes: {class: sel.substring(1)}};
  }
  else if (/^#/.test(sel)) {
    return {attributes: {id: sel.substring(1)}};
  }
  else if (/^\[/.test(sel)) {
    return {attributes: {[sel.substring(1, sel.length - 1)]: null}};
  }

  return {nodeName: sel}; //check against compiled name - only safe for native things like div

};
gnarf commented 7 years ago

Sure, but I'm not understanding why you'd resist the easy fix that works with all the current developer tools. React dev tools uses this display name, preact render to string also uses it.

Adding a display name to the component works for multiple tools, and the only reason it's not used much in the preact community yet is we have a lack of tools that do this stuff.

I agree that adding a functional selector to the where clause of preact render spy makes a lot of sense, but this doesn't work for higher order components in your library like withText

developit commented 7 years ago

@gnarf I merged #10 without even seeing this.

FWIW most JS engines should infer the name from a function declaration:

const Foo = () => {};
console.log(Foo.name);  // Foo

It seems like that should work fine with the condition here: https://github.com/mzgoddard/preact-render-spy/blob/cf3cb7401d09657b1d5073c02613369d651e501c/src/is-where.js#L12

The named function approach still breaks when minified though, which is an issue. For some components it's not super important, but I agree that things like <Text /> are useful to see as <Text /> instead of the more confusing <t /> when minified. I cut ~200b off this lib in #10 that would more than account for the bit of weight if we added displayName properties to the important components.

billneff79 commented 6 years ago

closing assuminging PR #10 fixed this issue. We can reopen if necessary