Skip to content
This repository was archived by the owner on Dec 7, 2021. It is now read-only.

js-ignore option#878

Closed
christophe-g wants to merge 12 commits intoPolymer:js-ignorefrom
christophe-g:js-ignore
Closed

js-ignore option#878
christophe-g wants to merge 12 commits intoPolymer:js-ignorefrom
christophe-g:js-ignore

Conversation

@christophe-g
Copy link
Copy Markdown
Contributor

@christophe-g christophe-g commented Sep 8, 2017

Adding a way to skip files in js minification process.

Should help addressing issues like #701.

Example (will skip all *.min.js files and firebase-database.js):

  "builds": [{
    "bundle": true,
    "js": {
        "minify": true ,
        "ignore": [
            "*.min.js",
            "firebase-database.js"
      ]}
  }],
  • CHANGELOG.md has been updated

@jsilvermist
Copy link
Copy Markdown
Contributor

Would a regex like that be maintainable for larger projects compared to something like an array which is used for other comparable project settings in polymer-cli? eg:

"js": {
  "minify": true,
  "ignore": [
    "*.min.js",
    "firebase-database.js"
  ]
}

Also, is this something that should be set and repeated in each build or set globally for all builds?

@christophe-g
Copy link
Copy Markdown
Contributor Author

Would a regex like that be maintainable for larger projects compared to something like an array which is used for other comparable project settings in polymer-cli?

I find regex to be more practical here - but open to change this to an Array if it gets more upvotes.

is this something that should be set and repeated in each build or set globally for all builds?
Merge state

It is a per build config.

@mgibas
Copy link
Copy Markdown

mgibas commented Sep 8, 2017

I really like wildcards/regex, specially for things like *.min.js.

@eskan
Copy link
Copy Markdown

eskan commented Sep 8, 2017

+1

@christophe-g
Copy link
Copy Markdown
Contributor Author

PR updated based on @jsilvermist suggestion

@JoelCode
Copy link
Copy Markdown

In the mean time, my solution around it is to do an extra step to copy/overwrite from the source, to the build directory after the build process is completed.

@christophe-g
Copy link
Copy Markdown
Contributor Author

@usergenic - thanks for 1.5.5 release.

Any chance this one gets reviews for the next one ?

@jogibear9988
Copy link
Copy Markdown

@jogibear9988
Copy link
Copy Markdown

can this be merged? as it seems to be the only usable fix for #886 and #633

@web-padawan
Copy link
Copy Markdown

@jogibear9988 I was wondering if this should ever be fixed on the polymer-cli side? Wouldn't it be better to file corresponding upstream issues to babili?

Anyway, I must say I don't use CLI for app projects due to the lack of flexibility and extending its configuration options looks good to me.

@jogibear9988
Copy link
Copy Markdown

don‘t know in which part of the cli this happens.

if it‘s a babili error, then it should be fixed there! maybe it‘s already fixed there? (don‘t know if polymer cli uses newest version) (newest name is babel minify)

@web-padawan
Copy link
Copy Markdown

The original issue should be revised once #847 gets merged. /cc @TimvdLippe

@christophe-g
Copy link
Copy Markdown
Contributor Author

@usergenic - any chance to get this reviewed ? thanks

@jogibear9988
Copy link
Copy Markdown

I've tested after the merge of #847, JQuery for example is still broken when minimized via polymer-cli

@jogibear9988
Copy link
Copy Markdown

jogibear9988 commented Oct 13, 2017

@usergenic @FredKSchott can this be reviewed? this is a really needed fix. without that many can not minify

@jogibear9988
Copy link
Copy Markdown

@justinfagnani

@christophe-g
Copy link
Copy Markdown
Contributor Author

@usergenic @FredKSchott @justinfagnani

Any chance to get this reviewed ?

  • without it, my app will not minify (it is not just Polymerfire, try to run a minify build against webassembly code)
  • 500% performance gain when excluding all already minified resources (js-ignore: ["*.min.js"]).

@jogibear9988
Copy link
Copy Markdown

The Team was triggered to look at that issue, see: Polymer/polymer#4931

@jogibear9988
Copy link
Copy Markdown

@christophe-g can you rebase so it compiles again?
@TimvdLippe ping

@christophe-g
Copy link
Copy Markdown
Contributor Author

@jogibear9988 - sure, done.

@jogibear9988
Copy link
Copy Markdown

jogibear9988 commented Nov 26, 2017

@christophe-g i think most of the issues why this is needed will be fixed in next version of babel minify (see babel/minify@04e4679 and babel/minify#733), but I think this should be merged (so I have a quick solution if a file could not be minifyed) ... @TimvdLippe said here Polymer/polymer#4931 we should ping this PR if we get no response from the team ....

don't know how to ping...

@christophe-g
Copy link
Copy Markdown
Contributor Author

@jogibear9988 - thanks!

I am very much in doubt next version of babel minify will solve all issues I currently have building my app, without the need of excluding some of the sources.

For instance, some modules are compiled versions of C/C++ libraries (http://kripken.github.io/emscripten-site) that make polymer-cli crash with a fatal error, similar to https://github.com/Polymer/polymer-cli/issues/623.

@laurentpellegrino
Copy link
Copy Markdown

@jogibear9988 ping
@TimvdLippe ping

@laurentpellegrino
Copy link
Copy Markdown

laurentpellegrino commented Jan 6, 2018

I encountered the same issue as @christophe-g . Building polymer-cli from this PR and using the new JS ignore option was a solution to make building work with both already minified and non-already minified JS files. Getting this PR merged or having another officially supported way to fix the original issue would be nice.

@hjr265
Copy link
Copy Markdown

hjr265 commented Jan 21, 2018

@usergenic Any update regarding this? It would be nice to have this merged or something similar implemented in the CLI.

@usergenic
Copy link
Copy Markdown
Contributor

Hey everyone, sorry for the delay on this. Reviewing this today. Seems like a useful if not necessary feature.

@jogibear9988
Copy link
Copy Markdown

We should also check to switch to babel minify nightly, because it has many bugs fixed

@usergenic usergenic changed the base branch from master to js-ignore January 22, 2018 20:06
@usergenic
Copy link
Copy Markdown
Contributor

Okay, reviewing this, there are some test failures here and the proposed changes technically require an update to polymer-project-config which expresses the configuration file format. I added this issue: https://github.com/Polymer/polymer-project-config/issues/50 and will work on getting in a PR for that today as well as a new PR for the CLI to operate on it.

Thanks for your patience, everyone.

@usergenic
Copy link
Copy Markdown
Contributor

Reimplemented as #952. Note exclude paths are not glob-enabled but rather act as prefixes so you can exclude folders etc.

@usergenic
Copy link
Copy Markdown
Contributor

On second though, I have added back matcher for wildcards. Now *.min.js works.

@usergenic
Copy link
Copy Markdown
Contributor

Also note that #952 does not have a command-line switch as part of the update. Use polymer.json build config for that or we can file subsequent PR for an override flag.

@jogibear9988
Copy link
Copy Markdown

Can you also look to switch to babel minify nightly, cause many of the issues because this is needed are fix in the nightly version. And it does not seem that they'll create a new release!

@christophe-g
Copy link
Copy Markdown
Contributor Author

closing as implemented by #952. @usergenic - thanks for lifting up the original idea!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.