Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX] Support ecmaVersion 2022 #259

Merged
merged 1 commit into from
May 27, 2024
Merged

Conversation

chienandalu
Copy link
Member

Odoo officially supports ecmaVersion 2022 from v17: https://github.com/odoo/odoo/blame/9a11717c17b860ec2f1b2e228517c0d3945c474a/addons/web/tooling/_eslintrc.json#L9

Possible fixes:

  • Support it globally
  • Make an special template for Odoo >= 17

What do you think?

@pedrobaeza
Copy link
Member

Please do it only for 17.0

@pedrobaeza
Copy link
Member

I think you should limit the previous template, and please also check #258

@chienandalu
Copy link
Member Author

I think you should limit the previous template, and please also check #258

That issue states that the new templating is used from eslint 9.3.0, but version 8.24.0 is being used right now.

{%- set repo_rev.eslint = "v8.24.0" %}

@pedrobaeza
Copy link
Member

Yes, but the idea is that you also update eslint in this shot (if there's no incompatibility).

@chienandalu
Copy link
Member Author

Yes, but the idea is that you also update eslint in this shot (if there's no incompatibility).

Let my try it locally anyway

@chienandalu chienandalu force-pushed the ecmaVersion-2022 branch 2 times, most recently from 0ab05f7 to 0d22ac6 Compare May 23, 2024 11:19
@chienandalu
Copy link
Member Author

Let my try it locally anyway

Well, I'm finding some issues locally with how the new eslint config works in a modular way... I can't figure out how to make it work, I'm afraid.

How's that an issue in any case? Isn't it just a nice to have?

@pedrobaeza
Copy link
Member

Yes, it was just a PoC. Then continue with the current esLint or bump it to the last compatible one.

@chienandalu chienandalu force-pushed the ecmaVersion-2022 branch 3 times, most recently from 4c4105b to be99b9d Compare May 23, 2024 11:33
@chienandalu
Copy link
Member Author

Ok, tried locally and working

@pedrobaeza
Copy link
Member

@sbidoul @yajo can you check?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like both eslintrc files are pretty similar. Couldn't you just name this file {% if odoo_version > 12 %}.eslintrc.yml{% endif %}.jinja (notice the .jinja suffix) and template whatever you need inside of it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read correctly the previous version, it was changing the ecmaVersion for all the versions, not only v17. Maybe I was wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, you both are right 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean that you can put:

  ecmaVersion: {% if odoo_version < 17 %}2019{% else %}2022{% endif %}

In a single file and avoid having 2.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done :)

Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Code review only.

@OCA-git-bot
Copy link

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you merge this and issue a new version? This can be applied massively to 17.0 branches. It shouldn't be too much problem, as we come from a massive update recently.

@sbidoul sbidoul merged commit 7ae1dbd into OCA:master May 27, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants