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

Bundled version mismatched with source #2232

Closed
starsolaris opened this issue Oct 2, 2018 · 7 comments
Closed

Bundled version mismatched with source #2232

starsolaris opened this issue Oct 2, 2018 · 7 comments
Labels
Type: Bug For bugs and any other unexpected breakage

Comments

@starsolaris
Copy link

Bundled version of mithril.js from next branch has some code mismatch with source:
https://github.com/MithrilJS/mithril.js/blob/next/mithril.js
line 1143:
key !== "href" && key2 !== "list" && key2 !== "form" && key2 !== "width" && key2 !== "height"// && key2 !== "type"
in method
function hasPropertyKey(vnode, key2, ns)

but should be key2 !== "href"

like in source render/render.js

@starsolaris starsolaris changed the title Bundled version mismatch with source Bundled version mismatched with source Oct 2, 2018
@pygy
Copy link
Member

pygy commented Oct 2, 2018

Ooof... The bundler strikes again :-/

Great catch @starsolaris! Thanks for the report...

... though I wish this hadn't popped up, I haven't been in the bundlers for eons, and while it may be short, it is anything but simple...

@pygy pygy added the Type: Bug For bugs and any other unexpected breakage label Oct 3, 2018
@spacejack
Copy link
Contributor

Is this the only issue blocking a V2 release?

Should we try using rollup instead of bundler?

@dead-claudia
Copy link
Member

@spacejack I'd love to switch this to Rollup, but I'd rather not block v2 on this.

@spacejack
Copy link
Contributor

@isiahmeadows I'm wondering what's the "least blocking" of options - fix bundler or switch to Rollup?

@dead-claudia
Copy link
Member

Neither is really "breaking", just it needs fixed, and switching to Rollup would likely be a lot more time-consuming than just fixing the bundler.

@spacejack
Copy link
Contributor

spacejack commented Oct 13, 2018

That's what I meant, which would take more effort. Are there difficulties in using Rollup beyond making a config? EDIT: Found this comment so I guess it is more involved than just "adding rollup".

@spacejack
Copy link
Contributor

Changing hasPropertyKey in render.js to:

function hasPropertyKey(vnode, key, ns) {
	// Filter out namespaced keys
	return ns === undefined && (
		// If it's a custom element, just keep it.
		vnode.tag.indexOf("-") > -1 || vnode.attrs != null && vnode.attrs.is
		// If it's a normal element, let's try to avoid a few browser bugs.
		|| key !== "href" && key !== "list" && key !== "form" && key !== "width" && key !== "height"// && key !== "type"
		// Defer the property check until *after* we check everything.
	) && key in vnode.dom
}

Prevents it from triggering the bundler bug. I'm at a loss trying to figure out bundler's logic for name mangled replacement though.

porsager added a commit to porsager/mithril.js that referenced this issue Oct 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug For bugs and any other unexpected breakage
Projects
None yet
Development

No branches or pull requests

4 participants