Invalidate raw components as valid nodes. #1005

dead-claudia · 2016-03-30T06:31:41Z

Edit:

Note that m(component, {key: 1}, arg0, arg1) now returns an object of the following structure:

{
    tag: component,
    args: [{key: 1}, arg0, arg1],
    attrs: {
       key: 1 // or `null` if none was provided
    }
}

Previously, it returned a component that looked like this:

{
    controller: function () { ... },
    view: function (ctrl) { ... },
    attrs: {key: 1} // This only exists if a key was passed.
}

This fixes #995.

I also converted the isolation test to Mocha to help me test that easier as well while writing this. It works in Firefox and Chrome, as well as old WebKit (well...Mocha mysteriously thinks there's a leaked i, but I highly doubt it's Mithril doing that).

As for performance, the TodoMVC benchmarks clock roughly a 5-10% speed improvement on Chrome and similar on Firefox. Also, the GC is a bit less. I didn't tune much outside what I had changed, though.

/cc @lhorie @tivac

dead-claudia · 2016-03-30T06:48:57Z

Oh, and as a drive-by, I fixed the controllers to always be called with new, to match the documentation. I didn't update the docs with this commit, only the code.

As for #994, that can be trivially implemented, since all controller instantiation follows the same code path. I just didn't want to decide that yet, and just fix a place where documentation didn't match reality.

dead-claudia · 2016-03-30T06:56:33Z

mithril.js

@@ -1959,9 +1988,17 @@
 		}
 	}

+	function isNativeError(e) {


Another drive-by. It was originally in the failed v0.2.1, and since disappeared.

It will break across frames.

@pygy

I don't think it would be safe to check that without exposing easy potential for XSS and CORS vulerabilities. If an error occurs in a child iframe, and it was constructed with new, it is instanceof the error's constructor within that frame. If an attacker defines a malicious getter on window and then throws an error that is instanceof the result of that getter, in hopes it gets caught, we've got a huge potential vulnerability. As a concrete example:

// In the <iframe> var oldTypeError = TypeError Object.defineProperty(window, "TypeError", { get: function () { initVirusDownload(); // Let's make it still seem like it's the real thing. return oldTypeError; }, set: function (value) { // Let's make it still seem like it's the real thing. oldTypeError = value; }, })

Also, I don't know of any reliable way to get the currently executing iframe, and I don't want to. It would also open up a whole class of XSS and CORS vulnerabilities by pure necessity.

I'd prefer not to open that can of worms.

The current method works across frames. Why do you want to change it?

Part of the conversation got lost in the GitHub Web UI:

On Wed, Mar 30, 2016 at 10:27 PM, Pierre-Yves Gérardy pygy79@gmail.com wrote:

On Wed, Mar 30, 2016 at 9:34 PM, Isiah Meadows notifications@github.com wrote:

@pygy This is for m.deferred.onerror(e).

Do you expect all these errors to come strictly from the main window?

I replaced that comment, because I wasn't thinking.

As for the change, it's going to be pulled out of this one.

dead-claudia · 2016-03-30T07:03:25Z

test/svg.html

@@ -206,7 +206,8 @@
 			transform: "translate(0,0)",
 		}, m("div", {xmlns: "http://www.w3.org/1999/xhtml"}, [
 			m.trust("this is a piece of html rendered as " +
-				"<a href=\"http://www.w3.org/TR/SVG11/extend.html\">" +
+				"<a target=\"_blank\" " +


Drive-by fix. The behavior didn't match what the page said it was supposed to be.

tivac · 2016-03-30T21:11:48Z

I don't have strong feelings on this beyond disliking the unrelated changes you've added. Those should be separate PRs.

dead-claudia · 2016-03-30T21:18:43Z

@tivac I'm not unfixing the sometimes-construct problem (which is inconsistent with the docs), because it's a little too tied to the rest of my patch. The rest I can remove, though.

Also, as a drive-by fix, always call controllers with `new`, as per documentation. I chose this way because of the unified code path, and going with the docs is the most conservative choice.

tivac · 2016-03-30T21:35:40Z

I've never knowingly run into the sometimes-new stuff, but I've looked at the code and seen that it could happen. I don't have a problem w/ that one.

dead-claudia · 2016-03-30T21:38:46Z

@tivac The previous implementation was only observable in ES5 for edge case stuff. ES6 is when people got concerned, since it was very observable when using ES6 classes (hence #994 and the two pens there). Thankfully, whatever ends up the case with #994 is a trivial fix after this patch.

dead-claudia · 2016-04-02T02:58:39Z

Note that I just updated the PR description.

…lidate-raw-component

dead-claudia · 2016-04-11T20:48:15Z

mithril.d.ts

@@ -528,15 +552,6 @@ declare namespace Mithril {
 	}

 	/**
-	* This represents a controller function.


This may reappear after #994 gets resolved (hopefully ASAP).

lhorie · 2016-04-11T20:54:08Z

Hey, sorry I forgot to add a comment here.

So, I went ahead and tried to update some project to this, but it actually turned out it was going to require a somewhat big migration effort in almost all of them.

The general challenge is that code using old school style component instantiation breaks. I had suspected that it would be possible to support both new and old component node structures, but if this requires a breaking change, I'm not convinced that the pros outweigh the cons. Basically, I know of several projects that would break with this change (some of which don't even have a maintenance retainer), but this fix wouldn't really have an upside for me (both in terms of code written and future code).

dead-claudia · 2016-04-11T21:03:32Z

@lhorie That doesn't bode well for your rewrite... 😦

I did tag this as 0.3 for a reason - I knew it would break things, but I wasn't sure of the severity. If it takes a while for users to be fixed, I can wait. Mind if I leave this open for the time being? I'll try to keep it in sync with next.

dead-claudia · 2016-04-11T21:13:49Z

@lhorie I could modify this to allow both, but it would slow things down somewhat. It would also complicate the PR as well, quite possibly increasing the diff by about 50%.

dead-claudia · 2016-06-16T17:12:16Z

Closing. If this ends up in an update between now and the rewrite's release, it can be rewritten against next of then.

dead-claudia mentioned this pull request Mar 30, 2016

Remove redundant generics, improve accuracy and completeness of types #997

Merged

dead-claudia force-pushed the invalidate-raw-component branch from d7bea5f to b724be7 Compare March 30, 2016 06:45

dead-claudia added this to the 0.3 milestone Mar 30, 2016

dead-claudia added enhancement Type: Breaking Change For any feature request or suggestion that could reasonably break existing code labels Mar 30, 2016

dead-claudia reviewed Mar 30, 2016
View reviewed changes

dead-claudia force-pushed the invalidate-raw-component branch from e032088 to 5e4998b Compare March 30, 2016 07:02

dead-claudia reviewed Mar 30, 2016
View reviewed changes

Invalidate raw components as valid nodes.

ec745a8

Also, as a drive-by fix, always call controllers with `new`, as per documentation. I chose this way because of the unified code path, and going with the docs is the most conservative choice.

dead-claudia force-pushed the invalidate-raw-component branch from 5e4998b to ec745a8 Compare March 30, 2016 21:34

dead-claudia force-pushed the invalidate-raw-component branch from bf61149 to 8b04965 Compare April 11, 2016 20:46

Merge branch 'next' of https://github.com/lhorie/mithril.js into inva…

464917d

…lidate-raw-component

dead-claudia force-pushed the invalidate-raw-component branch from 8b04965 to 464917d Compare April 11, 2016 20:47

dead-claudia reviewed Apr 11, 2016
View reviewed changes

dead-claudia closed this Jun 16, 2016

dead-claudia deleted the invalidate-raw-component branch June 16, 2016 17:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Invalidate raw components as valid nodes. #1005

Invalidate raw components as valid nodes. #1005

dead-claudia commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

dead-claudia Mar 30, 2016

pygy Mar 30, 2016

dead-claudia Mar 30, 2016

pygy Mar 30, 2016

dead-claudia Mar 30, 2016

dead-claudia Mar 30, 2016

tivac commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

tivac commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

dead-claudia commented Apr 2, 2016

dead-claudia Apr 11, 2016

lhorie commented Apr 11, 2016

dead-claudia commented Apr 11, 2016

dead-claudia commented Apr 11, 2016

dead-claudia commented Jun 16, 2016

Invalidate raw components as valid nodes. #1005

Invalidate raw components as valid nodes. #1005

Conversation

dead-claudia commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

dead-claudia Mar 30, 2016

Choose a reason for hiding this comment

pygy Mar 30, 2016

Choose a reason for hiding this comment

dead-claudia Mar 30, 2016

Choose a reason for hiding this comment

pygy Mar 30, 2016

Choose a reason for hiding this comment

dead-claudia Mar 30, 2016

Choose a reason for hiding this comment

dead-claudia Mar 30, 2016

Choose a reason for hiding this comment

tivac commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

tivac commented Mar 30, 2016

dead-claudia commented Mar 30, 2016

dead-claudia commented Apr 2, 2016

dead-claudia Apr 11, 2016

Choose a reason for hiding this comment

lhorie commented Apr 11, 2016

dead-claudia commented Apr 11, 2016

dead-claudia commented Apr 11, 2016

dead-claudia commented Jun 16, 2016