trivago / melody Goto Github PK

Explain the problem

Twig's set tag can be used to capture chunks of text (see https://twig.symfony.com/doc/2.x/tags/set.html) but Melody throws an error.

Expected Behaviour

Melody should not throw an error, and the output between the set and endset tags should be contained within the variable name as defined within the set tag.

Actual Behaviour

Melody throws an error: Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got undefined

Steps to reproduce

Define a Twig file that contains:

{% set test = 'boo' %}

{% set test %}
hi there
{% endset %}

{{ test }}

Explain the problem

Currently it is possible to use JS functions (e.g. parseInt {% for i in range(1, parseInt(data)) %}) in twig templates

Expected Behaviour

The build should fail or at least output a warning

Actual Behaviour

Builds successfully without any warning

I have a list twig:

<ul>
  {% for item in list %}
      {% mount './item' with {
        id: item.id
        text: item.text
      } %}
  {% endfor %}
</ul>

And an item twig:

<li key="li{{id}}">{{text}}</id>

This doesn't seem to work, except if I move the li tag to the list twig.
What is the preferred way to solve this?

Explain the problem

When trying to mount a root component written using the melody-hooks API, we still need to rely on melody-component in order to render the root component.

Expected Behaviour

There is a way to render a top-level component from melody-hooks without having to rely on melody-component.

import { render } from 'melody-hooks';
import home from './home';

const documentRoot = document.getElementById('root');
render(documentRoot, home, {
    message: 'Welcome to Melody!'
});

Actual Behaviour

The only way to render a top-level melody-hooks component is by using render from melody-component.

import { render } from 'melody-component';
import home from './home';

const documentRoot = document.getElementById('root');
render(documentRoot, home, {
    message: 'Welcome to Melody!'
});

Provide your Environment details

• melody version: 1.2.0-0

This happens in the nav menu of the documentation.

A bar appears on top of the "Advanced" header

Explain the problem

• When there is merge in master, actions runs twice. Ref: https://github.com/actions/bin/issues/36
• when pull request is opened from fork, then PR branch is not checked out. More info https://github.com/gr2m/git-checkout-pull-request-action
• On pull request, Action status is not visible, but they run in background #122 😮

Todo

• Also in release action, move up filter commit message level 1
• Filter PR on node6,10,11 action
• Add https://github.com/actions/bin/tree/master/filter#merged filter
• Adds coveralls (coverage) for github actions

Expected Behaviour

Not required

Actual Behaviour

Not required

Steps to reproduce

Not required

Provide a repository that reproduces issue if possible

Not required

Provide your Environment details

Not required

Explain the problem

Local development
It usually is a good idea to create a dummy repository to run your changes.

Expected Behaviour

With this issue I want to purpose a new way for testing melody most recent version against real sample applications.
The idea is basically for us to provide a set of self contained melody applications inside melody repository (like a portfolio), something like:

.
├── samples --- melody top level dir, real applications folder
├────── app1
├────── app2
├── bench
├── bin
├── node_modules
├── packages
├── rfcs
└── testsetup

What we would gain from this?

• A better way to perform e2e testing against melody most recent version making it easy to identify backward compatibility issues that may rise upon new developments.
• An integrated development experience since we wouldn't need to set up a separate repository to link melody and test it, we would have this within the same repository, out of the box (picked from CONTRIBUTING.md Local development section).
• Better documentation. What better to show newcomers how to use melody than a set of different real sample applications with varied complexity?

(NOTE: an example of this concept may be found in the angular/angular repository)

Explain the problem

travis fails on some tests for node 8

Explain the problem

At the moment, there's no extension for prettier which would allow it to prettify twig/melody templates.
All the reasons for why it makes sense to use prettier also apply for why Melody should be supported by it.

Expected behaviour

When I run prettier on a Melody file, it prettifies the template. It should also work regardless of which Melody plugins I am using (even if they introduce new AST nodes).

Hint for a solution

The Melody compiler is generally built to facilitate an external traversal strategy. However, for a tool like prettier that'd not work. The prettier plugin would have to include every existing Melody plugin or would itself need to be extensible through plugins (meaning you'd create plugins for a plugin).
Instead, I'd suggest to directly add it to Melodys Node class (https://github.com/trivago/melody/blob/master/packages/melody-types/src/index.js#L25) and to provide specializations for each and every node within Melody. Plugins will need to be updated to support prettier as well.
That way, going forward prettier will be a first class citizen and new language features will include a discussion on their formatting rules right away.

We'd have two options with respect to the release containing this:
a) We accept that its a breaking release and name it 2.0
b) We architect the solution in a way that will cause the prettier run to fail (with a nice error message) when a Node doesn't support prettification (breaking the prettier plugin) and are free to name it 1.x. At the point when we do create the 2.0 release, we'd be able to drop compatibility and require prettier support on node level for all plugins.

I'd prefer option b.

IntelliSense for melody is really hard to read in Visual Studio Code. VS Code generates this from JS Doc or Type Definition files.

Sadly, there are only very few places in melody that have JS Doc comments.
For some reason, a lot of functions have a license header instead of actual documentation.
Why do you write license header for every function instead of just once per file?

Those few parts that have JS Doc don't seem to show in IntelliSense either.. I guess VS Code can't make it work for node-modules. But Type Definition files do work and they even work for JavaScript, so your users don't need to use TypeScript to benefit from this.

A website with tutorials is of course a nice starting point, but when I code, I usually use IntelliSense instead of visiting a website.

As of my knowledge, currently melody parser does not have a warning method. It's good to implement a warn method just like error method in melody-parser.

When installing Melody (i.e. through create-melody-app) a bunch of warnings about invalid peer dependencies is generated.

warning "[email protected]" has incorrect peer dependency "melody-idom@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-idom@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-parser@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-runtime@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-traverse@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-types@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-idom@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-parser@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-runtime@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-traverse@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-types@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-types@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-types@^0.10.0".
warning "[email protected]" has incorrect peer dependency "melody-types@^0.10.0".

Explain the problem

The usage of dispatchCustomEvent accepts 3 arguments:

1. eventName
2. details
3. options

I think that in almost every case dispatchCustomEvent will be used for communicating with a parent component and therefore we need to set the bubbles: true option manually.

I searched in our current codebase for usage of dispatchCustomEvent:

• 6/6 use bubbles: true
• 4/6 use cancelable: true

Expected Behaviour

I would like to see at least the bubbles: true as a default option. Maybe it makes sense to set cancelable: true as well.

Actual Behaviour

Set all options manually inside the options object.

Explain the problem

We originally split Melody into lots of packages. While technically pretty it is questionable whether it actually delivered anything apart of complexity.

As part of this proposal, we'd reduce the amount of packages down to a more manageable size.

The Goal

Our users should be able to install and update Melody with at most 3 packages:

• The loader for their build system (Webpack / Rollup)
• The runtime (melody-idom, Preact, React, Inferno)
• The component system (melody-component, melody-hooks)

Details would still need to be discussed.

Explain the problem

If an img has an empty alt passed through the attrs filter, it does not get added because the filter doesn't add attributes with empty values.

Expected Behaviour

The alt attribute should be added with an empty string as its value.

Actual Behaviour

The alt attribute does not get added.

I think the attrs filter needs to add a special case so that it adds the alt attribute even if the provided value is an empty string (which is valid and often desired).

Is there a way to have some kind of mapping or resolution strategy in melody for looking up stuff like DefaultBundle:component:home.html.twig or something for include and extend and so on?

By now ES modules are a well supported standard and can be used by the browser as well as Webpack, Rollup and other bundlers. Melody itself doesn't benefit from being exported as CJS.

We can simplify our tooling and avoid mistakes for our users by switching to ES modules and ignoring the CJS use case.

This is a breaking change.

Explain the problem

Right now it is possible the access the props' current value by calling props.value.
This might lead to bugs where a property is updated, but we still have an old reference to it.
We should find a way to deny access to this property.

// Dangerous!
const List = ({ props }) => {
    const { items } = props.value;
    return combine({
        items
    });
};

// Correct way
const List = ({ props }) => {
    const items = props.pipe(pluck('items'));
    return combine({
        items
    });
};

I've noticed a fishy line in idom today while checking something else:

// packages/melody-idom/src/attributes.ts

if (typeof value === 'function') {
            let useCapture = name !== (name = name.replace(/Capture$/, ''));
            name = name.toLowerCase().substring(2);
            if (value) {
                if (!attrs[name])
                    el.addEventListener(name, eventProxy, useCapture);
            } else {
                el.removeEventListener(name, eventProxy, useCapture);
            }
            (el._listeners || (el._listeners = {}))[name] = value;
}

Because typeof value === 'function', if (value) is always true. Therefore it will never go into the else condition in which the event handler is removed.

Explain the problem

Child components aren't mounted if the document root in the HTML file uses another HTML tag than the root component.

Example:

<!-- index.html -->

<body>
   <div id="root"></div>
</body>

<!-- rootComponent.twig -->

<section>
   <h1>I am root 🌳</h1>
   {% mount '../childComponent' as 'childComponent' %}
</section>

// index.js

const documentRoot = document.getElementById('root');
render(documentRoot, rootComponent);

Expected Behaviour

I would expect that the child components are mounted properly or a warning/error should be shown.

Actual Behaviour

The root component renders as expected but instead of mounting the child component a <m-placeholder></m-placeholder> is rendered.

Steps to reproduce

1. Use Create Melody App
2. Change the outer <div class="home">...</div> in src/home/index.twig to something else e.g.
   <section class="home">...</section>
3. src/counter is not mounted

Provide your Environment details

• melody version: 1.2.0-21.2

Explain the problem

When the getFocusPath fn is called passing the node argument an svgElement the contains method is not part of it/

Expected Behaviour

Should return if node is on the root element or not (Boolean).

Actual Behaviour

It throws an error and break page.

Provide your Environment details

• Node version:
  Node 8.11
• Operating System:
  Windows 7/8 - IE11
• melody version:
  1.04

Explain the problem

At some point during Melodys existence we switched from Mocha & Chai to Jest. However, we did not update the existing tests to move away from Chai. This increases complexity when maintaining the tests and especially not using Snapshot tests where they'd be useful increases the cost of making changes.

Proposal

Update all tests to rely on Jest instead of having a mix between Chai & Jest. After that we should migrate tests that make sense to Snapshot tests.

Todo

• Replace Chai matchers with Jest
• Update to Snapshot tests

Explain the problem

With the new melody-streams in v1.2.0-21.0, child components are not rendered anymore for the first time by the parent when properties are equal. This is caused by incompatibility of streaming components and the current render queue.

Expected Behaviour

Child component should render for the first time when parent component pushes properties.

Actual Behaviour

Child component is not updating/rendering because the streaming api handles rendering itself and melody dequeues the child from rendering.

• melody version:
  v1.2.0-21.0

Explain the problem

When using template inheritance, Melody will omit anything except for top-level variable assignments (using {% set .... %}) that is not contained within a block.

Expected Behaviour

Melody should include all code that is non-rendering in its compilation.

Actual Behaviour

Melody only compiled top-level variable assignments.

Steps to reproduce

bar.html.twig

{% extends "foo.html.twig" %}

{% if bar %}
    {% set foo = "hello" %}
{% endif %}

foo.html.twig

{{ foo | default('Ciao') }}!!

Rendering bar.html.twig with bar set to true should emit hello!! but will instead emit Ciao!!.

Provide a repository that reproduces issue if possible

Provide your Environment details

• Node version:
  any
• Operating System:
  any
• melody version:
  any

Explain the problem

I started to use prettier-plugin-twig-melody to check my twig templates
and found that melody do not recognize the => Operator like it's used in the filter filter.

Expected Behaviour

Usage of the => operator like described in the filter filter.

Actual Behaviour

Throws Error:

ERROR in ./src/home/index.twig
ERROR: Unexpected token "operator" of value ">"
  16 |     {% mount '../counter' with { count: 5 } %}
  17 |     {% set sizes = [34, 36, 38, 40, 42] %}
> 18 |     {{ sizes|filter(v => v > 38)|join(', ') }}
     |                        ^
  19 | </div>

Is there a tutorial on how to write a custom Twig filter, or at least someone can give me some pointers on where to start?

Is it possible that, if I have product information for example, that gets fed into the twig template and expected as a 'product' variable in twig, to be available in melody as well?

When using melody-streams we sometimes get a very useful warning that says "Your component did not emit updates for 500ms".

Unfortunately when developing in the scope of a large application, it can be hard from time to time to understand where this warning is coming from.

Having the name of that component in the message would make this easier :)
Not sure about the internals but something like Function.name could come at hand.

Thanks!

Explain the problem

Attempting to dynamically include twig files throws an error. For example, through a variable:

{% set abc = 'location.melody.twig' %}
<div>
    {% include "./partials/#{abc}" %}
</div> 

Expected Behaviour

The file ./partials/location.melody.twig is loaded.

Actual Behaviour

Failed to compile error:

 ERROR in [...]/ItemDetails/index.melody.twig
    Module Error (from ./node_modules/melody-loader/lib/index.js):
    Property value expected type of string but got null

Steps to reproduce

Try to include a file determined through a variable.

Provide your Environment details

• Node version: 8.11.3

• Operating System: OSX Mojave

• melody version: 1.1.0

Explain the problem

Since node 7.x is EOL more and more packages dropping support for it. It will be a problem for us as well soon. We should drop support for it soon.

Provide a repository that reproduces issue if possible

#54 has an example for the problem

You can check 7.x EOL

Provide your Environment details

• Node version: 7

• Operating System: any

• melody version: any

Is there a way to make extensions/functions/filters work with melody as well?

I have an array of arrays with numbers in them in my state (top level array is called teams).
Imagine a twig template like this:

  <div id="teams">
    {% for team in teams %}
      <div>
        {% for team_member in team %}
          <div data-key="{{ team_member }}"><button ref="{{ moveTeamLeftButton }}">&lt;</button><span>{{ idToName[team_member] }}</span><button ref="{{ moveTeamRightButton }}">&gt;</button></div>
        {% endfor %}
      </div>
    {% endfor %}
  </div>

If I add class properties to either the wrapping team div (here showing without any properties) or to the team member div only the last element of the respective array is rendered, on the top level only the last array of ids, on the lower level only the last id.

Did I overread something about a key property like react has it or is the class property used for something special?

Hi.

Explain the problem

I found an Error when if and for statements have several close tags.

{% if product.stock > 10 %}
   <p>start P tag
{% elseif product.stock > 0 %}
   else if close P tag</p>
{% else %}
   else close P tag</p>
{% endif %}

⬇️

Error: ERROR: Unknown tag "elseif"
  3 | {% if product.stock > 10 %}
  4 |    <p>start P tag
> 5 | {% elseif product.stock > 0 %}
    |    ^^^^^^
  6 |    else if close P tag</p>
  7 | {% else %}
  8 |    else close P tag</p>

This sample is so simple, so it may not need to separate, but there are many complicated structure of DOM in production code like creating <li> element with parameters state.

Expected Behaviour

work correctly

Actual Behaviour

cause a ERROR: Unknown tag

Steps to reproduce

REPL is here.
https://repl.it/repls/SpectacularHarmlessOffices

Provide a repository that reproduces issue if possible

REPL is here.
https://repl.it/repls/SpectacularHarmlessOffices

Provide your Environment details

• Node version: v12.16.1

• Operating System: I don't know, sorry 😢

• melody version: latest

"dependencies": {
    "melody-extension-core": "^1.7.1",
    "melody-idom": "^1.7.1",
    "melody-parser": "^1.7.1",
    "melody-runtime": "^1.7.1",
    "melody-traverse": "^1.7.1",
    "melody-types": "^1.7.1"
  }

We need to setup:

• coveralls for melody similar to parallel-webpack
• A bot and/or a script to handle it.
• GitHub templates for issues/PR's
  -Contribution guidelines, etc.

@jamesb3ll @pago

example:

<div id="app">
  {% mount 'button.js' with {} %}
    {% block startAdornment %}
      <div .../>
    {% endblock %}
    {% block children %}
      <div .../>
    {% endblock %}
    {% block endAdornment %}
      <div .../>
    {% endblock %}
  {% endmount %}
</div>

or pass another component inside:

<div id="app">
  {% mount 'container.js' %}
    {% block children %}
      {% mount 'item.js' with {...} %}
      {% mount 'item.js' with {...} %}
      {% mount 'item.js' with {...} %}
    {% endblock %}
  {% endmount %}
</div>

or without block:

<div id="app">
  {% mount 'container.js' %}
    {% mount 'item.js' with {...} %}
    {% mount 'item.js' with {...} %}
    {% mount 'item.js' with {...} %}
  {% endmount %}
</div>

Explain the problem

Sometimes Melody removes an entire class listing from an element, causing styling issues.

Expected Behaviour

Melody shouldn't remove the class attribute from the element.

Actual Behaviour

Melody sometimes removes the class attribute

Steps to reproduce

It is difficult to reproduce as it doesn't happen consistently. I am not not sure under what conditions Melody does this.
But sometimes something like <span class="class1 class2>Hello there</span> is rendered as <span>Hello there</span>.
A reload will fix it, or using the classes filter: <span class="{{ { base: 'class1 class2' } | classes }}">Hello there</span>, but the classes filter shouldn't need to be used here as there are no conditional classes.

Provide a repository that reproduces issue if possible

N/A

Provide your Environment details

• Node version: v8.9.4

• Operating System: MacOS Mojave v10.14.1

• Melody version: v0.5.1

From the twig documentation, the ~ operator converts all operands into strings and concatenates them.

Melody compiles this to javascript +, which is fine in case at least one of the operands is a string, but not if they are both eg. numbers.

Both operands should be correctly converted to strings and then be concatenated.

Thanks :)

• melody version: 1.2.0

Explain the problem

If the stateReducer is added to createComponent I cannot enhance the component with bindevents anymore.

Expected Behaviour

Usage of stateReducer and bindEvents.

Actual Behaviour

No events are bound to the target.

Steps to reproduce

create a component with template and statereducers
add click bindevents
export the component to render it

Provide your Environment details

• Node version: 9.3.0

• Operating System: macOS 10.13.4

• melody version: latest

Explain the problem

Our current TypeScript integration is rather limited and dated. We should decide whether we want to keep using it (in that case: Switch to Babel TypeScript integration + ts-check) or if we consider it to be a failed attempt.

My personal preference would be to double down on TypeScript and to start converting our code base to fully utilize it. Especially a complex project like Melody which involves a parser, compiler and various transforms could benefit from type information and might be more approachable if we have that available.

Explain the problem
When the state field bound to an input is updated the second time to the same value, the value of the input element goes out of sync with the component state, seems like a caching issue. It seems that the first update of the field is reflected by the input's value, but the second time, as the field is updated to the same value, the DOM is not updated any more.

Expected Behaviour
The value of the input element should be updated every time the state is updated, and should not go out of sync from the component state.

Actual Behaviour
The second time the state is emitted with the same value for the input, the value of the input element does not change, and no longer reflects the value bound to it.

Steps to reproduce
Create a component where a field of the component state is bound to the value of an input element. Add logic that limits the number of the field (validation), which sets a predefined max or min number of the number manually set is out of bounds. Manually set a number lower or higher than allowed, so the state logic emits the same max or min number consecutively. The second time the same state value is emitted for that input, it will go out of sync.

Explain the problem

babel-preset-env is a new preset, first released over a year ago that replaces many presets that were previously used including:

• babel-preset-es2015, babel-preset-es2016, babel-preset-es2017
• babel-preset-latest
• other community plugins involving es20xx:
• babel-preset-node5, babel-preset-es2015-node, etc

Expected Behaviour

• Migrate to babel-preset-env

Actual Behaviour

Not required.

Steps to reproduce

Not required.

Provide a repository that reproduces issue if possible

Not required.

Provide your Environment details

• Node version:
  All

• Operating System:
  All

• melody version:
  All

The compiler should warn if a mount has no key:
{% mount 'component.js' %}
Should be:
{% mount 'component.js' as 'key' %}

Especially in for-loops mounts should have keys.

Also update the documentation and emphasise on this.

Coming from the React world we've got enzyme and react-testing-library to handle doing UI unit tests. Is there any equivalent libraries that allow someone to test interactions with the DOM after mounting a component?

I wa not able to find the answer in the documentations. If Melody support template sharing with server, how is it possible to render html on server and bind events on client?

Explain the problem

Creating a production release could be easier.

Expected Behaviour

Creating a release should be easy and nicely synced with Github.

Actual Behaviour

Creating a release is possible only for very few people and is complicated.

Further notes

• https://medium.com/devopslinks/automate-your-npm-publish-with-github-actions-dfe8059645dd

Explain the problem

We have some inconsistencies in the way how inputs (second argument of useEffect, useMemo, useCallback) are handled

Expected Behaviour

All hooks should have the same way of handling inputs. In React useEffect, useMemo, useCallback all have the same API:

// useEffect
// called on mount & update
useEffect(fn);
// called on mount
useEffect(fn, []);
// called on mount & when input values change
useEffect(fn, [a, b]);

// Internal implementation
const dirty =
    !inputs || (inputs.length && !shallowEqualsArray(dataPrev, inputs));

Actual Behaviour

// useEffect
// called on mount & update
useEffect(fn);
// called on mount
useEffect(fn, []);
// called on mount & when input values change
useEffect(fn, [a, b]);

// internal implementation
const dirty =
    !inputs || (inputs.length && !shallowEqualsArray(dataPrev, inputs));

// useCallback
// fn stored on mount (not stored on update! different from useEffect)
useCallback(fn);
// fn stored on mount
useCallback(fn, []);
// fn stored on mount & when input values change
useCallback(fn, [a, b]);

// internal implementation
const dirty =
    inputs && inputs.length && !shallowEqualsArray(inputsPrev, inputs);

// useMemo
// called on mount (not on update! different from useEffect)
useMemo(fn);
// called on mount
useMemo(fn, []);
// called on mount & when input values change, unmount
useMemo(fn, [a, b]);

// internal implementation
const dirty =
    inputs && inputs.length && !shallowEqualsArray(inputsPrev, inputs);

Maybe I'm missing something, but I think there was no good reason why these hooks have a different API. We should make them consistent. I'm happy to create a PR. What do you think?

In melody-parser, is method of babel-types has been called in a wrong way.

The is method signature is as follows which takes type:string and node:object:

function is(type, node, opts) {
  if (!node) return false;

  var matches = isType(node.type, type);
  if (!matches) return false;

  if (typeof opts === "undefined") {
    return true;
  } else {
    return t.shallowEqual(node, opts);
  }
}

Ref: is Method

but in melody-types

Explain the problem

Prefetch is causing a 404 error in some configurations with a CDN when prefetching starts too early before the correct CDN path was set.

Explain the problem

Fix peer dependency mismatch in melody packages

Provide your Environment details

• Node version: Not required

• Operating System:Not required

• melody version: 1.2.1

Ref: #143

Explain the problem

When there is a nonbreaking space (unicode character code 160) in a template, the Melody parser will output an "Unknown token" error message. In the console, this usually looks like a normal space, this is why it's confusing.

Expected Behaviour

It might be good to test explicitly for the nonbreaking space, and inform the developer-user of it.

Actual Behaviour

"Unknown token" with the arrow pointing to what looks like a normal space.

Steps to reproduce

Insert a nonbreaking space in any template and try to compile it.

Provide a repository that reproduces issue if possible

Provide your Environment details

• Node version:

• Operating System:

• melody version: