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

Delete React debugging props __self and __source #1690

Merged
merged 4 commits into from Jun 9, 2019

Conversation

mxstbr
Copy link
Contributor

@mxstbr mxstbr commented Jun 7, 2019

The @babel/preset-react preset includes two plugins which add __source and __self attributes to every element included development, which React uses for debugging. This muddles up the DOM and is pretty annoying.

For users of preact/compat, it is hard two remove these plugins as Babel does not allow disabling plugins included in a preset (see babel/babel#3016 (comment)). We could fork it, but unfortunately these plugins are included two levels deep so we would have to fork babel-preset-react-app and @babel/preset-react, and then keep updating them with upstream changes, which is a bit too much work for such a small change.

Instead, this is a tiny patch that (in development only) deletes these two attributes from every VNode if they exist. This should not impact production bundle size or performance, but would be a big win for the DX for our project.

Questions:

  1. Is this the right place to put the filtering? Should this live in preact/compat instead?
  2. Is the development check done correctly? I could not find any other instances of a NODE_ENV check outside of preact/debug, so I am not sure if that does what we need it to do.

Closes #1058

The @babel/preset-react preset includes two plugins which add `__source` and `__self` attributes to every element included development, which React uses for debugging. This muddles up the DOM and is pretty annoying.

For users of preact/compat, it is hard two remove these plugins as Babel does not allow disabling plugins included in a preset (see babel/babel#3016 (comment)), and forking `babel-preset-react-app` would mean having to keep updating it with upstream changes.

This is a tiny patch that, in development only, deletes these two attributes from every VNode if they exist. This should not impact production bundle size or performance, but would be a big win for the DX for our project.
@cristianbote
Copy link
Member

Hey @mxstbr 👋!

Thanks for taking the time to look and come up with a solution for this! 💯

Have you seen the options object from preact package, import { options } from "preact"?

The options object basically lets you jump into different phases of vnode creation and/or rendering. As you'll notice in here https://github.com/preactjs/preact/blob/master/src/create-element.js#L67 that hook is called, whenever a new vnode is created. So, one could easily define a vnode hook and handle whatever particular case one could find himself into. For example, if you already use the /compat that would require you to basically define a new .vnode hook - since there's one already defined for compat layer.

import { options } from "preact";

const old = options.vnode;
options.vnode = vnode => {
  // Call the previous 'hook'
  old(vnode);

  // Do the changes here
  delete vnode.props.__self;
  delete vnode.props.__source;
}

What do you think?

We do have a wip for integration with the devtools from React, @marvinhagemeister is working on that.

Thanks again for opening this! 👍

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 7, 2019

Ohh that works amazingly well, thank you, that solves my problem 100%! 🎉

We do have a wip for integration with the devtools from React, @marvinhagemeister is working on that.

Does that mean the __source and __self attributes will be used in the future and will be filtered anyway?

@cristianbote
Copy link
Member

Really happy that works for you 😄

Umm, I just checked with @marvinhagemeister, and seems like it's not part of the devtools integration. So, no, these won't be filtered by that, but the vnode hook will still work whatsoever.

@marvinhagemeister
Copy link
Member

marvinhagemeister commented Jun 8, 2019

@mxstbr Very cool too see you here 👍 💯

Yup, in the long-term we'd like to use both __source and __self for nicer error messages. We just haven't got around to it yet 🙂

Both properties should be only added during development, because from my understanding those babel plugins are not used in production. For that we stuff everything development related into preact/debug. We already remove __source from props there, but we missed __self.

cc @developit

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 8, 2019

Oh I see, I had not been using preact/debug! I thought that was only for the DevTools integration 🤔

So the right fix would be to add a filter for __self to preact/debug in the exact same manner that __source is filtered, am I understanding that correctly?

@mxstbr
Copy link
Contributor Author

mxstbr commented Jun 8, 2019

Implemented in the latest commit @marvinhagemeister—let me know if that looks better!

@coveralls
Copy link

coveralls commented Jun 8, 2019

Coverage Status

Coverage increased (+0.001%) to 99.776% when pulling 26a35f6 on mxstbr:delete-debugging-props into f90baf9 on preactjs:master.

@developit
Copy link
Member

Nice work all, thanks for the PR @mxstbr!

btw I think you make a good point - we need to clarify that the "elevator pitch" for preact/debug is similar to React's development version.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Thank you so much for the PR 👍 💯

@marvinhagemeister marvinhagemeister merged commit df27a6f into preactjs:master Jun 9, 2019
@mxstbr mxstbr deleted the delete-debugging-props branch June 9, 2019 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiled HTML looks faulty: __source / __self [object Object]
5 participants