posthtml / posthtml-expressions

Use variables, JS-like expressions, and even markup-powered logic in your HTML.
Other
123 stars 20 forks source link

strictMode: loops still throw error if target is not object #107

Closed cossssmin closed 3 years ago

cossssmin commented 3 years ago

https://github.com/posthtml/posthtml-expressions/blob/5a883311929ad53b44f3b51b9bd05af7f1eb0ef2/lib/index.js#L346-L349

@Scrum I think we could do a check for strictMode here?

- if (typeof target !== 'object') {
+ if (typeof target !== 'object' && opts.strictMode) {

Something like this will fail if items is undefined (or not an object for that matter), even though strictMode is set to false:

<if condition="items && items.length > 0">
  <each loop="item, key in items">
    {{ item.text }}
  </each>
</if>

Ref: https://github.com/maizzle/framework/issues/354

cossssmin commented 3 years ago

Maybe same here:

https://github.com/posthtml/posthtml-expressions/blob/5a883311929ad53b44f3b51b9bd05af7f1eb0ef2/lib/index.js#L385-L387

or here:

https://github.com/posthtml/posthtml-expressions/blob/5a883311929ad53b44f3b51b9bd05af7f1eb0ef2/lib/index.js#L391-L394

I guess what I mean is if strictMode: false, the plugin should be more forgiving of the user's mistakes and/or incorrect data types.

cossssmin commented 3 years ago

Hmm, just realized that this:

https://github.com/posthtml/posthtml-expressions/blob/5a883311929ad53b44f3b51b9bd05af7f1eb0ef2/lib/index.js#L344

... could use a try/catch to avoid breaking when evaluating code fails:

- const target = vm.runInContext(loopParams.expression, ctx)
+ const target = () => {
+   try {
+     return vm.runInContext(loopParams.expression, ctx)
+   } catch() {
+     return ctx
+   }
+ }

This way, we'd always return the ctx object, either ran in context or the original.

I think this might have to depend on strictMode though? I mean, the normal behavior should be "strict" and invalid code should break things, then strictMode: false could prevent it:

const target = () => {
  if (strictMode) {
    return vm.runInContext(loopParams.expression, ctx)
  } 
  else {
    try {
      return vm.runInContext(loopParams.expression, ctx)
    } catch() {
      return ctx
    }
  }
}

What do you think @Scrum?

EDIT: lol was writing at the same time, https://github.com/posthtml/posthtml-expressions/commit/d14fbbb683460fad4f8b2edb649f9ef759b89038 is perfect

Scrum commented 3 years ago

@cossssmin publish v1.6.2

schirliuradu commented 3 years ago

Still having issues with each loops, using posthtml inside maizzle email templating framework.

I'll leave a link to a repo that reproduces the bug here.

It would be enough to add extra condition here ( && opts.strictMode ) https://github.com/posthtml/posthtml-expressions/blob/bc94d52d3b496ded658cdc758eebe9cbed0ebc62/lib/index.js#L354

Maybe there would be even a better solution, but this seems to work as it should work.

cossssmin commented 3 years ago

@Scrum please consider @schirliuradu's comment above after merging #109, maybe we could include it and release a feature version.

Scrum commented 3 years ago

I'll leave a link to a repo that reproduces the bug here.

i couldn't deploy your project to get error Screenshot 2020-12-02 at 10 29 46

Scrum commented 3 years ago

It would be enough to add extra condition here ( && opts.strictMode )

@cossssmin I'll add this, but I need to understand the case to write tests for this.

Scrum commented 3 years ago

It would be enough to add extra condition here ( && opts.strictMode )

@cossssmin I'll add this, but I need to understand the case to write tests for this.

https://github.com/posthtml/posthtml-expressions/pull/110/commits/fc5662bff86f130506281fcaa12e5936ee8340a9

schirliuradu commented 3 years ago

Ok, i'll try to send a link to another repo, a bit later @Scrum

Scrum commented 3 years ago

Ok, i'll try to send a link to another repo, a bit later

I made a fix on posthtml-expressions try reinstall and try