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

lit-html render function breaks when minifying and compiling to es5 #724

Closed
orestisioakeimidis opened this issue Sep 26, 2018 · 16 comments
Closed
Labels

Comments

@orestisioakeimidis
Copy link

Hi,

I am not sure if this is the right place to report this issue, but since it is a problem with the code compilation, it seems wise.

You can see below the two versions of the compiled code for the render function of the lit-html package when using the Polymer LitElement.

ES5 (not minified)

function render(result, container) {
    var templateFactory$$1 = arguments.length > 2 && arguments[2] !== undefined ? arguments[2] : templateFactory;
    var part = parts$1.get(container);

    if (part === undefined) {
      removeNodes(container, container.firstChild);
      parts$1.set(container, part = new NodePart(templateFactory$$1));
      part.appendInto(container);
    }

    part.setValue(result);
    part.commit();
}

ES5 (minified)

function render(result, container) {
    var templateFactory$$1 = 2 < arguments.length && arguments[2] !== void 0 ? arguments[2] : templateFactory,
        part = parts$1.get(container);
    part.setValue(result);
    part.commit();
}

As you can see the check of the part is missing which results to the following error TypeError: Cannot read property 'setValue' of undefined at render.

These are the contents of the polymer.json file I am using.

{
  "npm": true,
  "lint": {
    "rules": [
      "polymer-3"
    ]
  },
  "entrypoint": "src/index.html",
  "shell": "src/main.js",
  "moduleResolution": "node",
  "extraDependencies": [
    "node_modules/@webcomponents/webcomponentsjs/**"
  ],
  "builds": [
    {
      "name": "esm-bundled",
      "browserCapabilities": [
        "es2015",
        "modules"
      ],
      "js": {
        "minify": true
      },
      "css": {
        "minify": true
      },
      "html": {
        "minify": true
      },
      "bundle": true
    },
    {
      "name": "es2015-bundled",
      "browserCapabilities": [
        "es2015"
      ],
      "js": {
        "compile": "es2015",
        "minify": true,
        "transformModulesToAmd": true
      },
      "css": {
        "minify": true
      },
      "html": {
        "minify": true
      },
      "bundle": true
    },
    {
      "name": "es5-bundled",
      "js": {
        "compile": "es5",
        "minify": true, // it works if I set this to false
        "transformModulesToAmd": true
      },
      "css": {
        "minify": true
      },
      "html": {
        "minify": true
      },
      "bundle": true
    }
  ]
}

Any ideas?

@Westbrook
Copy link

@justinfagnani I've just run into this as well. Have you see it, or have any thoughts on where to go with this?

My situation is as close to exactly the same as @orestisioakeimidis as possible. If having some sort of repo based reproduction would help find a resolution to this, let me know.

@orestisioakeimidis
Copy link
Author

Here is the repository to reproduce the error.

https://github.com/orestisioakeimidis/polymer-lit-element-repro

@web-padawan
Copy link
Contributor

Appears to be a babel-minify issue. Note there is new version 0.5.0 released recently.

@Westbrook
Copy link

Good looking out @web-padawan !

It looks like the commits that maintain that are from @usergenic 676c28c and @justinfagnani 7b77998

I'm gonna do some research and see if at least the issues faced here are addressed by unfixing those dependencies. Not sure if there were tests created to support those decisions that will be able to back up the reality of removing them or not...one thing at a time though!

@Westbrook
Copy link

On further review, it doesn't seem that updating those packages does the work we're looking for here.

However, in accordance with babel/minify#574 (comment) the addition of deadcode: false to packages/build/src/js-transform.ts in the babelPresetMinify settings on line 100 does bring back this code.

Trying to get tests to report actionable things about this locally, but might end up submitting s PR just to get those results easier. Theoretically, deadcode would only change some additional code being shipped down the wire, and nothing functionally, so it might be alright?

@Westbrook
Copy link

Further comparisons between my instance of the issue and the above description point to this specifically being about instances where options.compile === true || options.compile === 'es5'. Focusing the application of deadcode: false to that context is working well in application, the tests are running now, so hopefully that will go somewhere useful. However, that being so makes me wonder if minify as the cause here is a red herring and that there's something in this list of plugins that might be causing this... 😕

Continuing to look.

@web-padawan
Copy link
Contributor

Just in case if someone is interested: I have recently updated my Webpack starter to use Terser for minification: https://github.com/web-padawan/polymer3-webpack-starter

@orestisioakeimidis @Westbrook feel free to try the config from there on your projects and see if that works for you. I would be glad to hear any feedback (as my project does not yet include lit-html).

@LarsDenBakker
Copy link
Contributor

I have this issue as well. Any updates?

@LarsDenBakker
Copy link
Contributor

Changing

export function render(result, container, templateFactory = defaultTemplateFactory) {
    let part = parts.get(container);
    if (part === undefined) {
        removeNodes(container, container.firstChild);
        parts.set(container, part = new NodePart(templateFactory));
        part.appendInto(container);
    }
    part.setValue(result);
    part.commit();
}

to:

export function render(result, container, templateFactory = defaultTemplateFactory) {
    let part = parts.get(container);
    const isUndefined = part === undefined;
    if (isUndefined) {
        removeNodes(container, container.firstChild);
        parts.set(container, part = new NodePart(templateFactory));
        part.appendInto(container);
    }
    part.setValue(result);
    part.commit();
}

circumvents this particular issue. It's a bit scary, as this could happen to any part of the codebase.

@LarsDenBakker
Copy link
Contributor

LarsDenBakker commented Oct 10, 2018

After more digging, it seems that this got magically fixed in lit-html 0.12.x.

Before the render function was:

function render() {}

Now it's a variable:

const render = () => {}

This magically does not trigger the dead code removal, so it resolves the issue for now. But it can pop up in other places.

In the minimal reproduction repository linked above, the version of lit-element is clamped so you keep seeing the error there. Bumping the version resolves it.

@usergenic
Copy link
Contributor

Proposing #777

@usergenic
Copy link
Contributor

Also trying bumping babel-minify to more recent versions...

@usergenic
Copy link
Contributor

After bumping babel minify to latest version, I still encounter the error, where the container is not present in the parts$1 WeakMap:

screen shot 2018-11-02 at 2 28 21 pm

Turning off deadcode removal right now appears to be the only reliable approach.

@usergenic
Copy link
Contributor

More specifically I had updated the package.json with these two lines:

    "babel-plugin-minify-guarded-expressions": "^0.4.3",                                                                                                                                                                                                                                                                                                                  
    "babel-preset-minify": "^0.5.0",                                                                                                                                                                                                                                                                                                                                      

@usergenic
Copy link
Contributor

(the packages/build/package.json that is) and I even ran an npm run nuke to update all the package locks to latest versions of all-the-things but it didn't solve.

@stale
Copy link

stale bot commented Mar 4, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants