biotope / experimental-biotope-components

MIT License
2 stars 1 forks source link

move markup to a template file #20

Open timomayer opened 5 years ago

timomayer commented 5 years ago

i prefer to have the markup inside of a seperated template file, not inside the ts file (react jsx style)

https://github.com/biotope/experimental-biotope-components/blob/767984b90cd91f6bfb4c5d40e0875ae02fd50c24/src/components/Slider/Slider.ts#L61

what do you think? @tiagomapmarques @SheepFromHeaven @rhafner @marwesfur @jurekbarth @ctmm

marwesfur commented 5 years ago

worth a try to support it at least optionally, but that external file cannot simple be "html file" which is imported as string, as hyperHtml uses template literals (https://viperhtml.js.org/hyperhtml/documentation/#introduction-1)

lhwparis commented 5 years ago

Yes you are right @marwesfur but i think its worth a try because me (and i know many) FE devs hate JSX for the approach of this combined file. we are also a little bit in between already since we have the scss seperate but markup and js combined in one file

SheepFromHeaven commented 5 years ago

@timomayer we can try to do it, but it would still need to be a function returning js template literal as @marwesfur mentioned. Otherwise we will not be able to pass down attributes and event listeners directly from the js. All the data would need to be passed to the function then.

This would destroy our concept. I will give it a try and let's have feedback on the concrete code then.

SheepFromHeaven commented 5 years ago

I tried it:

Importing a template literal does not work, as it's being transformed to a string. Hyperhtml needs a template string, especially for the linking of events.

Is this a no go @timomayer ? This would mean we have to change the technological base away from hyperhtml.

tiagomapmarques commented 5 years ago

@marwesfur I agree on the approach. And I also agree on the file not being a simple "html file". What I propose is the following:

Our final "html" file (my-element.template.html):

<div>
  hello ${this.props.world}
</div>

The same file, converted by a custom -loader we write:

export default function() {
  return this.html`
<div>
  hello ${this.props.world}
</div>
  `;
}

Using it:

import template from './my-element.template';

export class MyElement extends BioElement<{ world: string }, {}> {
  static bioAttributes = ['world']

  constructor() {
    super();
    MyElement.renderTemplate = template;
  }
  // no render function needed
}

BioElement modification:

export abstract class BioElement ... {
  static renderTemplate: Function;

  created() {
    (this.constructor as any).renderTemplate = (this.constructor as any).renderTemplate.bind(this);
  }

  public render() {
    return (this.constructor as any).renderTemplate();
  }
}

What do you guys think?

SheepFromHeaven commented 5 years ago

@tiagomapmarques I also thought about this option. This would include a new way to think, as you will have to know what properties and functions are available where the template is being used.

I guess this would work but I don't like the developer experience.

If we see it as a "I totaly hate template literals so I will create a html file" approach, we add one more option which would take us away from having ONE way to work in biotope with every developer who has worked with it before.

If we see it as the standard way, I do not like this approach as explained above.

tiagomapmarques commented 5 years ago

Hence me doing a .bind(this) in the created function. This way, all variables in the class scope are available.

From what I can see, the developer experience is importing the file and setting a variable in a constructor.

As for the "one way of working with biotope", that's why I don't write the word function in my code anymore - I have a way of writing a function () => {}, I don't need a second (i.e. I'm completely on board with you on this one 😄). But I also agree with the concept of "separation of concerns". Html in a html file, js in a js file and css in a css file. So I'm of the opinion that we should move Biotope to this "one" way of thinking/programming.

SheepFromHeaven commented 5 years ago

@tiagomapmarques true, I never said it will not work 😉 I still have to know the interface of "this". So there is no autocomplete and markup errors in the html file.

So this is also just "separation of concerns" under the hood. You still are using JS template infusion.

Arguments to split it up:

Arguments to have the template literals:

@timomayer @tiagomapmarques please add arguments. I did not add separation of concerns, as this is already in there:

Logic out of the component, styling in scss, markup in the render function. It's just getting used to it.

tiagomapmarques commented 5 years ago

@SheepFromHeaven I edited your comment. I think having a centralised pro/cons section might help us.

As for my edit itself: again, because of separation of concerns, I dont think an html file (or string literal) is the place for logic. I believe logic should (to the best of our ability!) be put the js/ts file. I believe:

Html = information structure Css = visual structure Js/ts = funcionality and logic

jurekbarth commented 5 years ago

I don‘t like the custom loader as that makes everything special again. Bound to a build system means no easy switch to another buildsystem. So if there‘s not a super simple solution for that i would vote for no template file.

tiagomapmarques commented 5 years ago

@jurekbarth More than agree with you. Unfortunately, as far as I can see, no, there is no simple solution to this 😞. @marwesfur @SheepFromHeaven Any thoughts on how to do this in a simple way?

timomayer commented 5 years ago

@jurekbarth why do you think switching to another build system is important?

timomayer commented 5 years ago

i see the autocomplete argument @SheepFromHeaven so maybe a compromise: Tiagos suggestion but not with a seperate html file but a seperate js file for the template? we do not need the loader, we can import it. name could be something like component.tpl.js

tiagomapmarques commented 5 years ago

@timomayer I actually disagree with having a js file with a template. That will induce developers into writing logic on the template file :( hence me suggesting the loader thing.

But I agree with @jurekbarth that it is a very complicated solution to implement. If we end up using (for example) webpack and build a loader for it, then we can't easily switch to (for example) rollup.

[EDIT] Now that I think about it, I don't think I agree with type checking/safety being a fundamental part of the argument. If we end up "not letting" developers write logic in the template, the variables used in the template should all be trated as string and numbers (for text output), or "boolean-like" variables (for the if clauses) - which is basically what we want, right? On a personal note, this is kind of what frameworks do already. I've used Vue with typescript, and from what I've seen, they throw typechecking out the window on templates for (what I guess is) this exact reason - no logic in the template.

jurekbarth commented 5 years ago

@timomayer For me BioElement feels easy to use and lightweight. It‘s reusable and helpful for a lot of people i believe. So this would add a buildstep everytime i want to use it. Binding BioElement to a custom loader which itself is bound to a bundler. This means we and others cannot experiment without that loader. So it just doesn‘t feel right to me, if thats the only solution.
Back to your question being free of choice is a nice to have, that may not make sense for our usecase, but again at some point we would like to change and then we first need that loader working. For others that maybe a roadblocker to use BioElement.

timomayer commented 5 years ago

I prefer a template file without the ability to write logic in it for the exact same reasons than you do @tiagomapmarques, what I suggested was a compromise to bring it together, not my preferred solution :)

@jurekbarth i understand your point but like there are not so many frameworks, there are not so many build tools so maybe we could provide a webPack module doing this and a rollup one?

I think we have to decide between this:

  1. Template inside js file
  2. Template in html file
  3. We support both for bioElement
SheepFromHeaven commented 5 years ago

As just discussed with @timomayer the main reason agains is to prevent "real" logic to be pulled into the render function. It's also harder to review code in one file than in two.

What could be an option in my opinion is @tiagomapmarques proposal without the loader step:

export default function(render: HyperHtml, data: DataInterface) {
  return render`
    <div>
      hello ${data.world}
    </div>
  `;
}

Using it:

import template from './my-element.template';

export class MyElement extends BioElement<{ world: string }, {}> {
  static bioAttributes = ['world']

  constructor() {
    super();
  }

  render() {
     return template(this.html, {world: this.props.world})
  }
}

@tiagomapmarques @jurekbarth @timomayer what do you think?

timomayer commented 5 years ago

thanks @SheepFromHeaven think this is for now the best compromise we can get. We can also see if we wanna add the loader suggestion from @tiagomapmarques in a v2 or v3 if we see the benefits for developers.

tiagomapmarques commented 5 years ago

@SheepFromHeaven I only have a small concern regarding that implementation. Because hyperhtml does some variable binding magic behind the scenes to render the html faster, IDK if passing/remapping the data (return template(..., {world: this.props.world})) will have a negative effect on the component. Can you check if changing the prop world will at least trigger the render method?

SheepFromHeaven commented 5 years ago

@tiagomapmarques as the prop property is added completely by us, we are in total control of what happens. The only thing that HyperHtmlElement adds is the setState method which triggers the render method. As the instance of hyperHtml which gets passed as a parameter, the "binding-magic" will work anyways, as this happens on instance level!