marko-js / marko

A declarative, HTML-based language that makes building web apps fun
https://markojs.com/
MIT License
13.25k stars 643 forks source link

Top-level "if" statement in marko template yields unhelpful error #914

Closed brandones closed 6 years ago

brandones commented 6 years ago

Bug Report

Marko Version: 4.5.6

Details

I tried to create a component like

<if(input.flash)>
  <div class=`alert ${input.flash.type}`>
    ${input.flash.message}
  </div>
</if>

without wrapping it in a div.

Expected Behavior

An error like ERROR at filename.marko:1 : Marko directives must have an enclosing tag.

Actual Behavior

Uncaught TypeError: Cannot read property '___markoDetached' of null

Your Environment

Steps to Reproduce

  1. create a component with <if> as the top-level tag
  2. compile (should happen successfully)
  3. attempt to run in browser

Stack Trace

Uncaught TypeError: Cannot read property '___markoDetached' of null
    at morphComponent (VM28552 index.js:154)
    at morphChildren (VM28552 index.js:223)
    at morphComponent (VM28552 index.js:124)
    at morphChildren (VM28552 index.js:248)
    at morphdom (VM28552 index.js:517)
    at VM28553 Component.js:475
    at Object.batchUpdate [as ___batchUpdate] (VM28550 update-manager.js:63)
    at Component.___rerender (VM28553 Component.js:458)
    at initComponent (VM28556 init-components-browser.js:103)
    at VM28556 init-components-browser.js:261

DracotMolver commented 6 years ago

First:

have you tried you component on the online ? try online

try there and check what happens :)

brandones commented 6 years ago

Whoops, those versions are part of the "bug report" template. Updated to reflect the actual error environment -- Chrome Version 61.0.3163.100 (Official Build) (64-bit), Node 8.6.0.

DracotMolver commented 6 years ago

oooh lol ok ok. Can you share your code?. I will try it soon at home :)

DracotMolver commented 6 years ago

Well, I just did this: src/components/prueba-component/index.marko

$ var message = 'Hello World'

<if(input.flash)>
  <div class=`alert ${input.type}`>
    ${message}
  </div>
</if>

index.marko (./)

<test-component type='succes' flash=true/>

That worked perfect. No error.

If we find the ___markoDetached. You can see the next comment at the first checking of the value: Make sure we don't use a detached node as the component boundary and we can't use a node that is already the boundary node for another component

Definitely, I'd like to see your code.

austinkelleher commented 6 years ago

@brandones Are you still seeing this issue? I cannot reproduce it locally. Also we have tests for using <if> at the top level. Are you doing anything that would cause this node to get removed from the DOM? Maybe you could provide a sample project that reproduces the issue? Thanks!

Also thanks @DracotMolver for helping out. 👍 👍 👍

austinkelleher commented 6 years ago

@brandones I spent some more time this weekend trying to reproduce this issue and I still can't. Can you please provide a sample project or some additional code that reproduces this issue?

brandones commented 6 years ago

Yeah I've been totally unable to reproduce this in a toy project. Maybe I'll try making a "minimal" version of my existing app, where I'm hitting the problem.

austinkelleher commented 6 years ago

Thanks in advance @brandones!

austinkelleher commented 6 years ago

Also, if this issue is blocking you, try wrapping the <if> in a <div> and see if it goes away. This may be related to #920, which we are working on a fix for.

<div>
  <if(input.flash)>
    <div class=`alert ${input.flash.type}`>
      ${input.flash.message}
    </div>
  </if>
</div>
brandones commented 6 years ago

Yeah that's the workaround I'm using.

brandones commented 6 years ago

Ok, I filed down my app to something more minimalish. Here's a working example of the bug in action: https://github.com/brandones/marko-top-level-if

DracotMolver commented 6 years ago

@brandones Hi there. Well, It works for me. It isn't a Marko's bug. It's how you are instantiating the object in the state. You are using undefined and you should use an empty object ({ }). I don't know how Marko works about this. @austinkelleher could you give me some lights on this, please?. Becuase in plain JS it should works fine something like this:

var x = {
  z: undefined
};

x.z = {type:'tipo'}

// [object Object] {
//  z: [object Object] {
//    type: "tipo"
//  }
// }

I just know that using Object.freeze and object cannot change.

In my personal way, I never use undefined and hardly ever I do null. You could avoid them using !! if you are trying to check if the value coming is truthty. But, if you wanna have a instantiated value, try to add something more mmmm clear (Don't know how to explain myself here :(. sorry lol).

*your actual code

// ReportApp.marko
onInput(input, out) {
    this.state = {
       flash: undefined
   }
}

This works!

// ReportApp.marko
onInput(input, out) {
    this.state = {
       flash: {}
   }
}

Edited:

I think I found the answer lol. This is why you cannot use undefined :P. It's from where I found the comment of my early post above

...
if (startNode.___markoDetached !== undefined || startNode.___markoComponent) {
      startNode = insertBefore(createMarkerComment(), startNode, parentFromNode);
}

 if (endNode.___markoDetached !== undefined || endNode.___endNode) {
        endNode = insertAfter(createMarkerComment(), endNode, parentFromNode);
 }
...
brandones commented 6 years ago

Great, problem solved. Thank you for your help.

brandones commented 6 years ago

Actually, I'm gonna leave this open, because either way the error message could certainly stand to be improved upon.

austinkelleher commented 6 years ago

@brandones @DracotMolver This is a bug. You should definitely be able to so what you're trying to do. @DylanPiercey has spent some time working on a fix for this issue and #920. PR #927. We will get that merged soon and publish a new version.

Thanks again to both of you for working through this issue together and providing ample details. It helped a lot in creating a reproducible test case. Nice work. 👍 👍

DylanPiercey commented 6 years ago

@brandones this should be fixed in https://github.com/marko-js/marko/pull/927 and released as https://github.com/marko-js/marko/releases/tag/v4.6.0

Please feel free to ping me if the issue persists after updating.