zackad / prettier-plugin-twig

Code formatting plugin for Prettier to handle Twig templates
Apache License 2.0
30 stars 2 forks source link

Convert project to es modules #17

Closed rellafella closed 2 months ago

rellafella commented 2 months ago

This is unfortunately all in one giant commit, I noticed you already had a PR open for this so if this doesn't get merged I thought at least you might find some of the aspects useful.

My main issues were around getting jest to behave with es modules, from what I could tell generally people will keep the project type as commonjs when using jest, but they will just convert files to esm as they need to.

I have converted the test suite to use vitetest instead.

As far as updates go outside of the usual require and module.exports cleanup I have made some notes below where there have been any significant improvements.

zackad commented 2 months ago

LGTM. Test failed on prettier version 2 which personally I want to drop the support. Not sure what people think, should we keep compatibility with prettier 2 or drop it?

rellafella commented 2 months ago

I would rather see this move forward than have optimal backwards compatibility to be honest. However I might be able to take a look on the weekend to see what it would take to specify both an index.js and a index.cjs entry point. Pretty sure that would solve the install issue, but not sure that the code would be fully backwards compatible with common js, definitely not a module expert.

zackad commented 2 months ago

However I might be able to take a look on the weekend to see what it would take to specify both an index.js and a index.cjs entry point. Pretty sure that would solve the install issue, but not sure that the code would be fully backwards compatible with common js, definitely not a module expert.

The way I see it, we might need to add build step before publishing into npm registry. Similar to how trivago/melody handling support for commonjs and es module.

OTOH, there's problem with plugin system. Here what the output when loading switch-plugin

CommonJS

{% switch matrixBlock.type %}
    {% case 'text' %}
        {{ matrixBlock.textField|markdown }}
    {% case 'image' %}
        {{ matrixBlock.image[0].getImg() }}
    {% default %}
        <p>
            A font walks into a bar.
        </p>
        <p>
            The bartender says, “Hey, we don’t serve your type in here!”
        </p>
{% endswitch %}

ES Module

{% switch matrixBlock.type %}
{% case 'text' %}
    {{ matrixBlock.textField|markdown }}
{% case 'image' %}
    {{ matrixBlock.image[0].getImg() }}
{% default %}
    <p>
        A font walks into a bar.
    </p>
    <p>
        The bartender says, “Hey, we don’t serve your type in here!”
    </p>
{% endswitch %}

Still can't figure it out what the actual problem are.

rellafella commented 2 months ago

That's a good catch, i'll investigate here, I think this probably has something to do with this export, whilst being valid I don't think it is correct. https://github.com/zackad/prettier-plugin-twig-melody/blob/bcc1e020c5c0356e7d209e8e2bd7d9ac5faedb2e/src/plugins/switch-plugin/index.js#L33-L35

rellafella commented 2 months ago

I am going back over this with some proper attention to detail. Do you have a preference on using or avoiding default exports?

edit: actually never mind, I am going to keep this as like-for-like as possible. Anything named will stay named, anything default will stay as default.

zackad commented 2 months ago

I have preference to keep the changes to be as minimal as possible. Unfortunately your changes is not exactly compatible with existing plugin functionality as you can see from modified snapshot file.

Regarding plugin system, I'm not sure if I want to keep this feature. The switch-plugin is not a twig feature AFAIK. Personally I want to remove plugin system. Do people actually use it? What do you think?

I haven't test it on my local machine (hopefully this weekend).

rellafella commented 2 months ago

I am just working on it now actually stand by for some updates to this branch.

I think that is a valid suggestion to just scrap the plugin functionality or at least remove it for now, to perhaps add it in later if it is going to stall further development.

Whilst the switch-plugin isn't part of twig, it is part of Craft CMS (since at least 2.0.0) which is a common use case for this prettier plugin.

Out of curiosity what system are you using Twig for and does that come bundled with some custom Twig extensions that you want to include?

Craft CMS docs for tags

https://craftcms.com/docs/2.x/templating/switch.html https://craftcms.com/docs/3.x/dev/tags.html https://craftcms.com/docs/4.x/dev/tags.html https://craftcms.com/docs/5.x/reference/twig/tags.html

zackad commented 2 months ago

I'm using twig with symfony framework. Didn't know about twig use case outside symfony framework ecosystem. Knowing this, I don't want to break existing functionality (atleast without offering some alternatives).

For now I want to keep all features as-is. Extension/features I want to add in the future includes:

rellafella commented 2 months ago

I would say the easiest way would be to just scrap the plugin utility and move to an approach where that functionality is built into the core

rellafella commented 2 months ago

I have updated the PR with an example of what this would look like without the plugin feature and the switch tag print just rolled directly into the core

zackad commented 2 months ago

Thank you @rellafella