Polymer / tools

Polymer Tools Monorepo
BSD 3-Clause "New" or "Revised" License
430 stars 200 forks source link

excludes in JS minify being skipped #279

Open Westbrook opened 6 years ago

Westbrook commented 6 years ago

I've get a situation very close to https://github.com/Polymer/polymer-cli/issues/701 and while I've applied the approach added via https://github.com/Polymer/polymer-cli/pull/952 I'm noticing that the files in question are still being minified in a way that triggers errors in the Firebase code.

Tracking through the build/cli code I noticed that when the files are entering the getOptimizeStreams section of the build pipeline, the file was being tracked with a rewritten file.pathName such that file. relative would return an unmatchable file name when passed to notExcluded for comparison. The can be experienced via https://github.com/Westbrook/polymer-build-filenames which outlines the steps to see this in action.

Upon further review with some help from @justinfagnani I realized that all the the JS files, no matter their origin, were being seen as isInline === true at https://github.com/Polymer/tools/blob/master/packages/build/src/html-splitter.ts#L189 which would clearly cause this issue. It seems be happening because bundle is occurring before getOptimizeStreams here https://github.com/Polymer/tools/blob/master/packages/cli/src/build/build.ts#L84 It seems that this issue can be avoided by moving the if (bundled) { block to just before the if (options.basePath) { block here https://github.com/Polymer/tools/blob/master/packages/cli/src/build/build.ts#L128 and wanted to see if that made sense to someone more knowledgable on this project that I.

I'd be happy to PR this if you'd like, though I know there is lots of dev going against this right now. Either way let me know if I can help get the right fix for this into the codebase!

davie-robertson commented 6 years ago

I thought I was going mad - I've been facing the same issue for weeks.

https://stackoverflow.com/questions/50073467/

Justin - I think this is the same issue (or at least related) I asked you to take a look at (via twitter) regarding a lib (UnityLoader.js) that keeps getting screwed with in the build process (even taking the code from my custom element and incorporating in directly in my App now breaks during an ES5 build)

Westbrook commented 6 years ago

FYI, I'm still experiencing this issue with a fresh install of v1.7.3. I'd be happy to submit a PR if the code ordering was seen as an acceptable fix for this issue. Please advice.

iSuslov commented 5 years ago

Still the same in 1.8.0 and issue submitted in "old" repository. https://github.com/Polymer/polymer-cli/issues/1008 Poor documentation, and after one tries multiple approaches wasting time it turns out that it is simply not working.

So for anyone who needs excludes for js to work, use v1.6.0

christophe-g commented 5 years ago

Gentle ping.

Seems to be an important issue (... at least preventing me to upgrade).

christophe-g commented 5 years ago

@usergenic - any chance for the cli to honor minify.exclude as it used to ?

christophe-g commented 5 years ago

@usergenic - not giving up ; )

DrNiels commented 5 years ago

Exclusion still doesn't seem to work with 1.9.9. What is the status here? Is there some workaround for the time being? Or would I need to manually copy the excluded dependencies after build?

Westbrook commented 5 years ago

FYI: while I was able to get by with the code suggested in https://github.com/Polymer/tools/pull/584 I was never able to get the tests to pass and never did the work upgrade the pull to the current version of the CLI. I moved away from using polymerfire to rxfire for the project relying on this feature and have had good success. Particularly, polymerfire being a bit of a blocker to moving to Polymer 3/LitElement I was very happy with the flexibility I achieved with the change. I realize there are other libraries that might suffer from this issue, and in those cases (if you're on Polymer 3/LitElement already) it might be better to use Rollup/Webpack via the suggestions found here: https://open-wc.org/building/#using-index-html-as-an-entry-point

usergenic commented 5 years ago

Hey everyone, thanks for your patience. I'm working on getting this in right now.

usergenic commented 5 years ago

Okay so one issue with the PR moving the bundler to after the JS transformation stuff happens is that bundler doesn't know how to bundle anything which has been transformed to AMD.

While I investigate refactoring things, you might try adding the files you want to exclude to the config's bundle > excludes array as well as the js > minify > exclude array (and don't ask why one term is plural and one is singular) in your polymer.json:

{
  "npm": true,
  "moduleResolution": "node",
  "build": [
    {
      "name": "bundled-es",
      "bundle": {
        "excludes": [ "./path/to/whatever.js" ]
      },
      "js": {
        "minify": {
          "exclude": [ "./path/to/whatever.js" ]
        }
      }
    }
  ]
}
usergenic commented 5 years ago

The reason why the js.minify.excludes bit isn't working for you right now (it's not "broken" per se) is that when you're bundling, the bundles have already been constructed by the time the filenames are matched in the minifier against exclusion list. You can exclude the specific bundle from minification if you want if you know that bundles name and that should work (if not then it is a bug)

stale[bot] commented 4 years ago

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.