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

Remove atom-space-pen-views and custom elements from bundled packages #13254

Closed
as-cii opened this Issue Nov 17, 2016 · 17 comments

Comments

Projects
None yet
7 participants
@as-cii
Member

as-cii commented Nov 17, 2016

Refs: #13253

In order to snapshot bundled packages we need to remove eval time interactions with DOM APIs. The two biggest offenders in this regard are jQuery (which is required by atom-space-pen-views) and HTML custom elements. The issues with jQuery could be addressed by patching jQuery to not do feature detection, but considering that we want to remove it anyway over the long term and it causes other performance and complexity problems, it's probably cleaner to just remove jQuery completely. Custom elements were a failed experiment and we aren't actually getting value out of registering them with the DOM global. They can be replaced with plain JS objects managing a DOM element.

Affected packages

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 18, 2016

They can be replaced with plain JS objects managing a DOM element.

Will such objects be accessible from the DOM? For instance, the File instance of a FileView element can be looked up through the DOM's .file property. Having these objects exposed offers many footholds into runtime tweaking that wouldn't be possible if such objects were sealed from external access.

Glad to hear jQuery is being shown the door, BTW. =) Finally, no more element[0].

Alhadis commented Nov 18, 2016

They can be replaced with plain JS objects managing a DOM element.

Will such objects be accessible from the DOM? For instance, the File instance of a FileView element can be looked up through the DOM's .file property. Having these objects exposed offers many footholds into runtime tweaking that wouldn't be possible if such objects were sealed from external access.

Glad to hear jQuery is being shown the door, BTW. =) Finally, no more element[0].

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Nov 21, 2016

Member

Glad to hear jQuery is being shown the door, BTW. =) Finally, no more element[0].

👍

Will such objects be accessible from the DOM? For instance, the File instance of a FileView element can be looked up through the DOM's .file property. Having these objects exposed offers many footholds into runtime tweaking that wouldn't be possible if such objects were sealed from external access.

Yeah, we are thinking to add those properties to the DOM nodes managed by the plain JS objects. This way we should be able to make these changes and, at the same time, maintain backward compatibility.

Member

as-cii commented Nov 21, 2016

Glad to hear jQuery is being shown the door, BTW. =) Finally, no more element[0].

👍

Will such objects be accessible from the DOM? For instance, the File instance of a FileView element can be looked up through the DOM's .file property. Having these objects exposed offers many footholds into runtime tweaking that wouldn't be possible if such objects were sealed from external access.

Yeah, we are thinking to add those properties to the DOM nodes managed by the plain JS objects. This way we should be able to make these changes and, at the same time, maintain backward compatibility.

@simurai

This comment has been minimized.

Show comment
Hide comment
@simurai

simurai Nov 25, 2016

Member

Remove custom elements

Does this mean using divs instead? Like:

  • <atom-text-editor> 👉 <div class="atom-text-editor">
  • <atom-workspace> 👉 <div class="atom-workspacer">
  • <atom-pane> 👉 <div class="atom-pane">
  • etc.

I like custom elements because you can easily see where an element starts and where it ends, regardless of indentation. If everything is a div it will be harder to inspect. But that might not be a big enough reason to keep them just for that. 😬

Anyways, that would be quite a big change and I assume we would have to convert all the selectors like we did for the Shadow DOM removal?

Member

simurai commented Nov 25, 2016

Remove custom elements

Does this mean using divs instead? Like:

  • <atom-text-editor> 👉 <div class="atom-text-editor">
  • <atom-workspace> 👉 <div class="atom-workspacer">
  • <atom-pane> 👉 <div class="atom-pane">
  • etc.

I like custom elements because you can easily see where an element starts and where it ends, regardless of indentation. If everything is a div it will be harder to inspect. But that might not be a big enough reason to keep them just for that. 😬

Anyways, that would be quite a big change and I assume we would have to convert all the selectors like we did for the Shadow DOM removal?

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Nov 25, 2016

Member

Does this mean using divs instead? Like:

<atom-text-editor> 👉 <div class="atom-text-editor">
<atom-workspace> 👉 <div class="atom-workspacer">
<atom-pane> 👉 <div class="atom-pane">
etc.

Anyways, that would be quite a big change and I assume we would have to convert all the selectors like we did for the Shadow DOM removal?

Yeah I think this issue refers to packages, so the scope of the change would be narrower, but I believe it would be nice to stop using custom elements across the board and convert custom elements to divs in core as well. As you mentioned, this will require transforming style sheets but it should be relatively easy to detect atom-* element selectors and replace them on the fly with a class instead.

In this regard, I am more concerned about package authors instantiating custom elements via document.createElement or, worse, via HTML. We might probably monkey patch the former, but it might be very difficult to provide a shim for the latter. Exception made for <atom-text-editor>, however, I think that the instantiation of core and bundled packages custom elements is not very widespread in third party packages, so we could be able to deprecate them without waiting till 2.0. TextEditorElement will probably have to wait until then, unless we can submit and merge pull requests to all the affected packages.

I like custom elements because you can easily see where an element starts and where it ends, regardless of indentation. If everything is a div it will be harder to inspect. But that might not be a big enough reason to keep them just for that.

That's a good point. 👍 Still, I think that losing a little bit of clarity in the dev tools is a reasonable tradeoff compared to registering an element globally (especially since it gets in the way of using it during the snapshot phase).

Member

as-cii commented Nov 25, 2016

Does this mean using divs instead? Like:

<atom-text-editor> 👉 <div class="atom-text-editor">
<atom-workspace> 👉 <div class="atom-workspacer">
<atom-pane> 👉 <div class="atom-pane">
etc.

Anyways, that would be quite a big change and I assume we would have to convert all the selectors like we did for the Shadow DOM removal?

Yeah I think this issue refers to packages, so the scope of the change would be narrower, but I believe it would be nice to stop using custom elements across the board and convert custom elements to divs in core as well. As you mentioned, this will require transforming style sheets but it should be relatively easy to detect atom-* element selectors and replace them on the fly with a class instead.

In this regard, I am more concerned about package authors instantiating custom elements via document.createElement or, worse, via HTML. We might probably monkey patch the former, but it might be very difficult to provide a shim for the latter. Exception made for <atom-text-editor>, however, I think that the instantiation of core and bundled packages custom elements is not very widespread in third party packages, so we could be able to deprecate them without waiting till 2.0. TextEditorElement will probably have to wait until then, unless we can submit and merge pull requests to all the affected packages.

I like custom elements because you can easily see where an element starts and where it ends, regardless of indentation. If everything is a div it will be harder to inspect. But that might not be a big enough reason to keep them just for that.

That's a good point. 👍 Still, I think that losing a little bit of clarity in the dev tools is a reasonable tradeoff compared to registering an element globally (especially since it gets in the way of using it during the snapshot phase).

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 25, 2016

In this regard, I am more concerned about package authors instantiating custom elements via document.createElement or, worse, via HTML. We might probably monkey patch the former, but it might be very difficult to provide a shim for the latter.

Both can be addressed pretty cleanly using a MutationObserver:

const watcher = new MutationObserver(mutations => {
	mutations.forEach(mutation => {
		for(const node of mutation.addedNodes){
			if(/^\w+-\w+/.test(node.nodeName)){
				Grim.warn("Stop this shit. Seriously.");
			}
		}
	});
})

watcher.observe(document.documentElement, {
	childList: true,
	subtree: true
});

Monkey-patching anything is bad, especially when it comes to web/DOM APIs.

Alhadis commented Nov 25, 2016

In this regard, I am more concerned about package authors instantiating custom elements via document.createElement or, worse, via HTML. We might probably monkey patch the former, but it might be very difficult to provide a shim for the latter.

Both can be addressed pretty cleanly using a MutationObserver:

const watcher = new MutationObserver(mutations => {
	mutations.forEach(mutation => {
		for(const node of mutation.addedNodes){
			if(/^\w+-\w+/.test(node.nodeName)){
				Grim.warn("Stop this shit. Seriously.");
			}
		}
	});
})

watcher.observe(document.documentElement, {
	childList: true,
	subtree: true
});

Monkey-patching anything is bad, especially when it comes to web/DOM APIs.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Nov 25, 2016

Member

@Alhadis Looks like that would have a significant performance cost in comparison to a (less-than-ideal) monkey patch or shim. In this context performance is paramount, even for a short-lived piece of code necessary during a deprecation period.


I haven't actually tested my claim, but gut feeling from seeing nested loops, with the inner loop containing a regex.

Member

thomasjo commented Nov 25, 2016

@Alhadis Looks like that would have a significant performance cost in comparison to a (less-than-ideal) monkey patch or shim. In this context performance is paramount, even for a short-lived piece of code necessary during a deprecation period.


I haven't actually tested my claim, but gut feeling from seeing nested loops, with the inner loop containing a regex.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 25, 2016

It's not looping through literally every element in the DOM. It's only dispatching events whenever an element is added (or removed). addedNodes is a NodeList of every element that's being inserted, which will be 2-3 at most.

The overhead of monitoring mutation events is probably greater than the overhead spent testing a regex (especially since the latter can be optimised by V8, whereas the former is DOM-driven, so no optimisation is possible).

Alhadis commented Nov 25, 2016

It's not looping through literally every element in the DOM. It's only dispatching events whenever an element is added (or removed). addedNodes is a NodeList of every element that's being inserted, which will be 2-3 at most.

The overhead of monitoring mutation events is probably greater than the overhead spent testing a regex (especially since the latter can be optimised by V8, whereas the former is DOM-driven, so no optimisation is possible).

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Nov 25, 2016

Member

Both can be addressed pretty cleanly using a MutationObserver:

As far as I understand, MutationObserver notifies listeners after the fact, which means we would still have to register the element for the HTML use case. This is even more evident with document.createElement, as it happens before mutating the DOM (which might also not happen at all). What am I missing?

Monkey-patching anything is bad, especially when it comes to web/DOM APIs.

Totally, I was suggesting it as it seemed the only way we could transition away from custom elements without going to 2.0. Anyways, I feel like we should focus just on packages and think about core later: <atom-text-editor> is probably the only custom element that's used in the way I described above, so it might be fine to leave it alone until we reach 2.0.

Member

as-cii commented Nov 25, 2016

Both can be addressed pretty cleanly using a MutationObserver:

As far as I understand, MutationObserver notifies listeners after the fact, which means we would still have to register the element for the HTML use case. This is even more evident with document.createElement, as it happens before mutating the DOM (which might also not happen at all). What am I missing?

Monkey-patching anything is bad, especially when it comes to web/DOM APIs.

Totally, I was suggesting it as it seemed the only way we could transition away from custom elements without going to 2.0. Anyways, I feel like we should focus just on packages and think about core later: <atom-text-editor> is probably the only custom element that's used in the way I described above, so it might be fine to leave it alone until we reach 2.0.

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 25, 2016

No, a MutationObserver can be registered before packages are loaded, and it persists in the session until its disconnect method is called (which is essentially like calling removeEventListener).

Alhadis commented Nov 25, 2016

No, a MutationObserver can be registered before packages are loaded, and it persists in the session until its disconnect method is called (which is essentially like calling removeEventListener).

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 25, 2016

@as-cii It may help to think of a MutationObserver as a sort of privileged addEventListener. It responds to specific changes (which are stated in the second argument passed to its observe method), so it's not making an effort to fire a handler in response to literally every change in the DOM.

It may help to limit the scope of its observation to elements outside the text-editor, if you're concerned about performance. It will, after all, be running on virtually every keystroke.

Alhadis commented Nov 25, 2016

@as-cii It may help to think of a MutationObserver as a sort of privileged addEventListener. It responds to specific changes (which are stated in the second argument passed to its observe method), so it's not making an effort to fire a handler in response to literally every change in the DOM.

It may help to limit the scope of its observation to elements outside the text-editor, if you're concerned about performance. It will, after all, be running on virtually every keystroke.

@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Nov 25, 2016

Member

Leaving performance aside, I believe MutationObserver is fundamentally the wrong primitive to solve this problem. To recap, the problem we want to solve is removing all calls to document.registerElement from core and bundled package. If we did that, calls like document.createElement('atom-text-editor') and HTML fragments like <atom-text-editor> would stop working because that particular identifier (atom-text-editor) wouldn't exist anymore.

MutationObserver seems the wrong facility to use because of two reasons:

  1. It doesn't catch calls to document.createElement, because they don't mutate the DOM.
  2. For the HTML scenario, the event is triggered after creating/modifying/deleting nodes. In our example, however, <atom-text-editor> wouldn't be a known identifier for Chrome, which will likely throw an error saying the HTML fragment is invalid and never trigger the mutation observer's callback.
Member

as-cii commented Nov 25, 2016

Leaving performance aside, I believe MutationObserver is fundamentally the wrong primitive to solve this problem. To recap, the problem we want to solve is removing all calls to document.registerElement from core and bundled package. If we did that, calls like document.createElement('atom-text-editor') and HTML fragments like <atom-text-editor> would stop working because that particular identifier (atom-text-editor) wouldn't exist anymore.

MutationObserver seems the wrong facility to use because of two reasons:

  1. It doesn't catch calls to document.createElement, because they don't mutate the DOM.
  2. For the HTML scenario, the event is triggered after creating/modifying/deleting nodes. In our example, however, <atom-text-editor> wouldn't be a known identifier for Chrome, which will likely throw an error saying the HTML fragment is invalid and never trigger the mutation observer's callback.
@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis Nov 25, 2016

My bad, I misinterpreted. I thought the problem was reliably issuing deprecation notices for package authors; I overlooked the fact that this is supposed to intercept calls outright.

Okay, well, in that case, it's obvious that a monkey-patch is the solution. Sometimes you have no choice.

This is reminding me of something I did in late 2014 to see if I could get classList supported in IE6. I was laughing when it actually worked.

Alhadis commented Nov 25, 2016

My bad, I misinterpreted. I thought the problem was reliably issuing deprecation notices for package authors; I overlooked the fact that this is supposed to intercept calls outright.

Okay, well, in that case, it's obvious that a monkey-patch is the solution. Sometimes you have no choice.

This is reminding me of something I did in late 2014 to see if I could get classList supported in IE6. I was laughing when it actually worked.

@maxbrunsfeld maxbrunsfeld referenced this issue Jan 10, 2017

Merged

Remove the use of jQuery #838

7 of 7 tasks complete

Alhadis added a commit to file-icons/atom that referenced this issue Feb 6, 2017

@as-cii as-cii referenced this issue Mar 2, 2017

Merged

Startup Snapshot #13916

13 of 13 tasks complete
@as-cii

This comment has been minimized.

Show comment
Hide comment
@as-cii

as-cii Mar 14, 2017

Member

It looks like we've checked off all the items in the list, so I think we can mark this as completed!

Member

as-cii commented Mar 14, 2017

It looks like we've checked off all the items in the list, so I think we can mark this as completed!

@as-cii as-cii closed this Mar 14, 2017

@lgeiger lgeiger referenced this issue Apr 18, 2017

Open

Roadmap #657

13 of 17 tasks complete
@trusktr

This comment has been minimized.

Show comment
Hide comment
@trusktr

trusktr May 30, 2017

Is new vanilla DOM manipulation built from scratch? Or is it using some existing tool (f.e. React, Angular, etc)?

trusktr commented May 30, 2017

Is new vanilla DOM manipulation built from scratch? Or is it using some existing tool (f.e. React, Angular, etc)?

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu May 30, 2017

Member

jQuery was replaced mostly by a combination of etch and atom-select-list (which uses etch).

Member

50Wliu commented May 30, 2017

jQuery was replaced mostly by a combination of etch and atom-select-list (which uses etch).

@Alhadis

This comment has been minimized.

Show comment
Hide comment
@Alhadis

Alhadis May 30, 2017

Just wanna say thanks for not forcing us to use a framework. =) Seriously.

Alhadis commented May 30, 2017

Just wanna say thanks for not forcing us to use a framework. =) Seriously.

@lock

This comment has been minimized.

Show comment
Hide comment
@lock

lock bot Apr 1, 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 bot commented Apr 1, 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 1, 2018

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