-
Notifications
You must be signed in to change notification settings - Fork 94
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
[IMP] eslint: bump to escmascript 2019 #122
Conversation
Chosen as MAX_ES_VERSION by Odoo since 14.0 [^1] [^1]: https://github.com/odoo/odoo/blob/c16bc3192/odoo/addons/test_lint/tests/test_ecmascript.py#L14
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems legit. After all, the fact some Odoo version was released some years ago doesn't mean its uses still use the browsers that got released back then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR has the |
What do you think @Tardo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
Is this applying to all versions? If so, I think it should be limited to 14.0+ |
Hum... In another side, the risk is to make red a some branches (13.0 (and then 10 / 11 / 12, and #117 will be merged)). if this test fails on the 10.0 web or point-of-sale branch, there is little chance that maintainers will be motivated to correct it. So maybe better to add a "if v >= 14" test indeed, as @pedrobaeza proposed. @ivantodorovich : what do you think ? |
AFAIK increasing As a dev, it'd be a shame having to code in old js syntax, even for 13.0.. when the only limitation is |
Are you sure about this assert? I remember the Odoo JS framework limited in the past to use certain features of modern JS. |
Odoo Framework might not know how to handle new features, but that doesn't mean you can't use them. For instance, it's not this case, but when js introduced classes or promises.. old Odoo code won't know how to handle them, so you can't pass them around as arguments to its framework. But you can still use them in your code. On a general note, new js versions are always backwards compatible.. it would otherwise crash 99% of the internet if it weren't 😅 the only limit here is the browser's support |
OK, if you have it clear, I won't oppose. |
Enabling it for all versions may be misleading. I agree that it should be for >=13.0. For example, eslint should not allow the use of promises in older versions. It is tempting to mix promises and jquery deferred, but it would result in errors. The good thing for the current change is that this is already happening with 2017. On the other hand, Odoo <=11.0 uses phantomJS, using something greater than es7 without having it well bounded is dangerous (https://webkit.org/status/). That said, I don't see any problem in using the latest version of ecmaScript that can be used from Odoo version >=13.0. |
El mié, 16 de mar de 2022 a las 15:01:17 PM, Alexandre Díaz
***@***.***> escribió:
On the other hand, Odoo <=11.0 uses phantomJS, using something
greater than es7 without having it well bounded is dangerous
(<https://webkit.org/status/>).
Also, to run tests, you can't predict which chromium version is
installed alongside newer Odoo installations. Another point to keep in
mind.
|
Yes, that's a good point |
Since the template applies to versions >= 13 only, the will only run flake8+pylint for versions < 13 (not eslint etc) I understand the consensus is that this is same enough. So merging. Let me know if I misunderstood some arguments. |
Chosen as
MAX_ES_VERSION
by Odoo since 14.0 [^1]NOTE: IMO it's not worth adding version-specific templates for this (to keep 2017 version for 13.0), as the list of supported browsers hasn't changed.. it's only that we're all a bit older now