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 text editor to use the Shadow DOM #3677

Closed
benogle opened this Issue Sep 30, 2014 · 32 comments

Comments

Projects
None yet
@benogle
Copy link
Contributor

benogle commented Sep 30, 2014

Via @nathansobo

Status: Will be enabled by default in 0.166.0.

@benogle benogle added the in-progress label Sep 30, 2014

@benogle benogle referenced this issue Sep 30, 2014

Closed

API Freeze : Atom 1.0-pre #3041

18 of 21 tasks complete

@nathansobo nathansobo changed the title Update internal components to use the Shadow DOM Update text editor to use the Shadow DOM Oct 6, 2014

@nathansobo nathansobo added on-deck and removed in-progress labels Oct 6, 2014

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Oct 8, 2014

👍 Sounds awesome.

@matiaspunx

This comment has been minimized.

Copy link

matiaspunx commented Oct 8, 2014

Great!

@mark-hahn

This comment has been minimized.

Copy link
Contributor

mark-hahn commented Oct 8, 2014

Ditto on the Great!

Will it not use react any more?

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Oct 8, 2014

I'm planning to transition away from React to reduce the number of moving parts but it will probably be a separate unit of work.

@pboudoin

This comment has been minimized.

Copy link

pboudoin commented Oct 24, 2014

Sounds great, but be aware that currently shadow dom and custom elements can really slow down a webapp

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Oct 28, 2014

@pboudoin Do you have any links/resources for more information on that? So far we haven't seen a slowdown using custom elements in production, but we haven't rolled out the shadow DOM yet.

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 5, 2014

Closed via #3943

@benogle benogle closed this Nov 5, 2014

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Nov 5, 2014

@benogle Was thinking about leaving this open until we actually had the shadow DOM on permanently and the option removed. There could still be issues to resolve with it.

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 5, 2014

Ah ok.

@benogle benogle reopened this Nov 5, 2014

@bolinfest

This comment has been minimized.

Copy link
Contributor

bolinfest commented Nov 12, 2014

I am very concerned about the implications of this design decision. Let me explain.

Common knowledge has been that touching the DOM is generally the slowest thing you can do in a web browser. From High Performance JavaScript (O'Reilly 2010), p.35-36:

It's common across browsers to keep DOM and JavaScript implementations
independent of each other.
...
Inherently Slow
What does that mean for performance? Simply having two separate pieces
of functionality interfacing with each other will always come at a
cost...Every time your ECMAScript needs access to the DOM,
you have to cross this bridge and pay the performance toll fee.
The more you work with the DOM, the more you pay.
So the general recommendation is to cross that bridge as
few times as possible and strive to stay in ECMAScript land.

Then a bunch of graphs follow with various benchmarks demonstrating how slow it is. Because the book is from 2010, the data is from browsers like Chrome 3 and IE 8. They show how moving a call to getElementById() outside a loop makes their little piece of sample code 20-273x faster, depending on which browser was tested.

React gets this. It knows that it is cheaper to spend extra JavaScript cycles calculating what part of the DOM needs to be updated if it lets you reduce the amount you have to touch the DOM overall. The math works out because touching the DOM is just so expensive.

jQuery (on which SpacePen is built) does not get this. It touches the DOM with reckless abandon. In moving to a world in which using the Shadow DOM is the favored way to build UI in Atom, I fear this will only make things worse. For each individual button or menu, the costs may seem negligible, but when you put all of these components together, you may end up dying by a thousand cuts.

By comparison, if you went with React through and through, you would get something that is fast by default.

The data that I am citing is admittedly a bit dated, but I don't know whether the fundamental architecture of web browsers that is responsible for the performance penalty has changed much. I think Chrome was going to try to move more of the implementation of the DOM into JavaScript at some point, but that was a couple of Google I/Os ago. Arguably, Atom only cares about Chrome, though if Atom is ever going to be repurposed as a web application, it should care about how it would perform in all browsers.

In sum, I would do your homework on this one (i.e., run your own benchmarks), but death by a thousand cuts is my prediction if you go with the Shadow DOM. Many web developers are big fans of React (it's over 10,000 stars and counting on GitHub), so I wouldn't worry about it having any sort of adverse affect on how many developers contribute to and invest in Atom if you start to invest more in React. If anything, it will likely make the ecosystem more attractive because React will provide better scaffolding for building great UI.

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 12, 2014

We appreciate the concern. The shadow DOM does not really change anything we were doing before with respect to DOM updates. All it does is sandbox the styles and make for cleanliness while looking at the inspector. The DOM update mechanisms remain the same — it's even still using react.

By comparison, if you went with React through and through, you would get something that is fast by default.

This is not true by our testing. React was slower and more memory intensive than manual DOM manipulation in the lines component, so we needed to move to manual manipulation in componentDidUpdate.

@nathansobo has a lot of blood, sweat, tears, profiles and opinions in this architecture, and whats coming next. It's a little presumptuous to assume we have done none of our own "homework" whatsoever.

@postcasio

This comment has been minimized.

Copy link
Contributor

postcasio commented Nov 18, 2014

How do the shadow DOM changes affect custom styles? Is there a way to target elements in the shadow DOM from within ~/.atom/styles.less?

Edit: never mind, I'm assuming the ::shadow pseudo-element can be used.

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Nov 18, 2014

@postcasio You can also use /deep/ for things like scrollbar styles to target everything.

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 18, 2014

@postcasio We wrote some upgrade guides that attempt to cover these topics: https://github.com/atom/atom/tree/master/docs/upgrading would you mind taking a look? If you have feedback on them that would be

@postcasio

This comment has been minimized.

Copy link
Contributor

postcasio commented Nov 20, 2014

Looks pretty good to me. Maybe there should be a link from the styles.less section in https://github.com/atom/atom/blob/master/docs/customizing-atom.md to these docs.

@postcasio

This comment has been minimized.

Copy link
Contributor

postcasio commented Nov 20, 2014

Would it be worth having context targeting for user styles? So that you could have:

~/.atom/styles.less
~/.atom/styles.atom-text-editor.less

etc.

@cooolbasha

This comment has been minimized.

Copy link

cooolbasha commented Nov 20, 2014

How about using Polymer web components

@nickmccurdy

This comment has been minimized.

Copy link

nickmccurdy commented Nov 20, 2014

From my (limited) experience, it seems like Polymer would not have very good performance for something of this scale.

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Nov 20, 2014

The issue was more that Polymer makes heavy use of globals and seems to assume it owns the entire browser window. That's too big a commitment to make to any framework. After working really hard to remove jQuery, we want to keep Atom's core modular and agnostic going forward.

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 20, 2014

@nicolasmccurdy We did experiment with using polymer bits as a package view system. The integration work is at #3979 with an example package at https://github.com/benogle/template-explore and the extracted parts of polymer in https://github.com/atom/elmer. But as @nathansobo said, polymer is pretty heavy handed with the global pollution, so we decided against it for now.

@cooolbasha

This comment has been minimized.

Copy link

cooolbasha commented Nov 20, 2014

so you will be going with spacepen or custom element approach.

@giantelk

This comment has been minimized.

Copy link

giantelk commented Nov 22, 2014

To cut through the black ink on this page, are you guys scrapping ReactJS completely?

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Nov 23, 2014

so you will be going with spacepen or custom element approach.

are you guys scrapping ReactJS completely

We were considering making React the standard for all views, but upon further thought it doesn't make sense for a couple of reasons:

  • API compatibility: Say we bake in React 0.11 and tell everyone to write their views against that API. Then React 0.12 comes out with breaking changes. Should we upgrade and break all the existing views? Should we stay on the previous version and force new package authors to code to an outdated API. There's no good options.
  • Package author freedom: I wasn't happy that we were forcing package authors to interact with jQuery and SpacePen just to write packages. React is way better than SpacePen in many ways, but someday there may be something even better than React, and I want to give people freedom to use it. Also, different problems demand different solutions. We wanted a solution that left more freedom of choice for package authors.

For these reasons, we've settled on custom elements as our basic approach to view integration. That API is baked into the browser and super minimal. On top of that, a variety of different frameworks can be used. We can develop something more abstract in the future, or the community can come up with a solution. The point is that we'll have flexibility over the long term and won't paint ourselves into a corner using a specific framework.

@giantelk

This comment has been minimized.

Copy link

giantelk commented Nov 24, 2014

Nice answer. RE: we've settled on custom elements as our basic approach to view integration.

So kinda like Polymer?

@benogle

This comment has been minimized.

Copy link
Contributor Author

benogle commented Nov 24, 2014

So kinda like Polymer?

Kind of. But without all the helpers or custom element definitions in HTML for now.

@benogle benogle added shipping and removed in-progress labels Dec 15, 2014

@Stanzilla

This comment has been minimized.

Copy link

Stanzilla commented Dec 23, 2014

So is the usage of React going to be removed or while stay in for some views?

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Dec 24, 2014

React probably won't be used in the text editor long term, but could be used for specific components in packages. Nothing is set in stone right now, however. It may stay in, we just have to see how it goes.

@nathansobo nathansobo closed this Jan 6, 2015

@QuantumInformation

This comment has been minimized.

Copy link

QuantumInformation commented Mar 15, 2016

@nathansobo So do you find that using native js, is better overall than the "supposed" productivity gains that react gives developers?

@nathansobo

This comment has been minimized.

Copy link
Contributor

nathansobo commented Mar 15, 2016

@QuantumInformation I wouldn't say better overall. I'd say more appropriate for our use case in the editor.

@rolfen

This comment has been minimized.

Copy link

rolfen commented Dec 1, 2016

Thank you for the reasonable responses.
And for Atom as well!

@DemiMarie

This comment has been minimized.

Copy link

DemiMarie commented Feb 12, 2017

Did this cause a performance regression?

@lock

This comment has been minimized.

Copy link

lock bot commented Apr 3, 2018

This issue has been automatically locked since there has not been any recent activity after it was closed. If you can still reproduce this issue in Safe Mode then please open a new issue and fill out the entire issue template to ensure that we have enough information to address your issue. Thanks!

@lock lock bot locked and limited conversation to collaborators Apr 3, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.