web icon indicating copy to clipboard operation
web copied to clipboard

[15.0] [MIG] web_widget_model_viewer

Open andreampiovesana opened this issue 3 years ago • 15 comments

standard migration to 15

andreampiovesana avatar Nov 24 '22 21:11 andreampiovesana

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). 🤖

OCA-git-bot avatar Nov 29 '22 22:11 OCA-git-bot

merge?

andreampiovesana avatar Nov 30 '22 22:11 andreampiovesana

One of the approvals should be from a PSC. Why is this PR adding a JS file with 4200 new lines?

pedrobaeza avatar Dec 01 '22 07:12 pedrobaeza

we use google https://modelviewer.dev/ that use https://threejs.org/ is like web_timeline that use js libs ... this is a migration from v14

andreampiovesana avatar Dec 01 '22 13:12 andreampiovesana

But this is a migration. It shouldn't require new things. Are you updating the JS library and this new version requires an extra JS file? If that's the case, I recommend you to do it in 2 steps.

/ocabot migration web_widget_model_viewer

pedrobaeza avatar Dec 01 '22 14:12 pedrobaeza

I see in lib folder 2 files model-viewer-legacy.js and model-viewer.js. Why these 2?

pedrobaeza avatar Dec 01 '22 14:12 pedrobaeza

Yes, model-viewer.min.js should be deleted as it is not used anymore. It was the ESM version of the lib requiring a <script type="module" src="..."> to work, especially tricky with the Odoo standard assets bundler that will output regular script tags.

There are 2 solutions:

  • Doing like in version [14.0], extending web.layout and including the lib directly in the <head> tag with the correct type="module" directive. The problem with that is that now the lib will be present in the global JS, loaded also on the Website for example without being used.
  • Using the "legacy" transpiled version of the lib that can be added in the regular Odoo 15 assets.bakend keys in the __manifest__.py.

I personally prefer solution 2 so the module doesn't have an unexpected footprint on the front-end views.

PhilDL avatar Dec 01 '22 15:12 PhilDL

But Odoo v15 accepts new ES6 JS modules. They have to be put with .esm.js extension, but I have to admit that I don't know more internals, but I'm sure you don't need to do such hacks.

pedrobaeza avatar Dec 01 '22 15:12 pedrobaeza

Actually, with this change, there is no hack because the version of the lib is already the transpiled version and the JS lib is added in the assets like usual.

I am sorry but .esm.js is not natively accepted by Odoo 15, and unfortunately has no impact on how the Odoo Asset Bundler will minify, and create JS bundles.

The "transpiling" is done in python from all the JS added in manifests, you can see it here https://github.com/odoo/odoo/blob/15.0/odoo/addons/base/models/assetsbundle.py


class JavascriptAsset(WebAsset):

    def __init__(self, bundle, inline=None, url=None, filename=None):
        super().__init__(bundle, inline, url, filename)
        self.is_transpiled = is_odoo_module(super().content)
        self._converted_content = None

    @property
    def content(self):
        content = super().content
        if self.is_transpiled:
            if not self._converted_content:
                self._converted_content = transpile_javascript(self.url, content)
            return self._converted_content
        return content

And to know if a module should be transpiled just depends on either the JS file begins with /** @odoo-module **/ :

ODOO_MODULE_RE = re.compile(r"""
    \s*                                       # some starting space
    \/(\*|\/).*\s*                            # // or /*
    @odoo-module                              # @odoo-module
    (\s+alias=(?P<alias>[\w.]+))?             # alias=web.AbstractAction (optional)
    (\s+default=(?P<default>False|false|0))?  # default=False or false or 0 (optional)
""", re.VERBOSE)


def is_odoo_module(content):
    """
    Detect if the file is a native odoo module.
    We look for a comment containing @odoo-module.

    :param content: source code
    :return: is this a odoo module that need transpilation ?
    """
    result = ODOO_MODULE_RE.match(content)
    return bool(result)

Don't ask me why sometimes they use "*.esm.js" extension, as far as I know, it is used as a convention or maybe picked up by some .eslint rules to resolve imports differently when it is ES6 syntax.

PhilDL avatar Dec 01 '22 16:12 PhilDL

Yes, .esm.js is a convention for activating the proper linter, but I mentioned it as a hint for finding the mechanism that it's done, but if you have it clear that there's no other way, please do:

  • Remove the useless file.
  • Add this explanation on the migration commit to understand the big diff comes from this.

pedrobaeza avatar Dec 01 '22 16:12 pedrobaeza

Hello, I think everyone agrees on what's needed next:

  1. Delete model-viewer.min.js
  2. Edit the migration commit message to explain The model-viewer-legacy.js is using the "legacy" transpiled version of the lib that can be added in the regular Odoo 15

@andreampiovesana @PhilDL cc @pedrobaeza

dreispt avatar Nov 06 '23 18:11 dreispt

@andreampiovesana ping

simahawk avatar Nov 20 '23 08:11 simahawk

Hi there,

I noticed that the Pull Request for migrating the module to version 15 has been inactive for quite some time. I was wondering if there's anything I can do to help move this forward so we can take it later to the latest version 17.

JavierB7 avatar Apr 08 '24 20:04 JavierB7

you don't need, are already developed for version 16 and 17, if you want you can contribute making PR

Hi there,

I noticed that the Pull Request for migrating the module to version 15 has been inactive for quite some time. I was wondering if there's anything I can do to help move this forward so we can take it later to the latest vversion 17.

andreampiovesana avatar Apr 10 '24 12:04 andreampiovesana