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

Fix #827. Merge commandline options with named build configuration options #930

Closed
wants to merge 1 commit into from

Conversation

alber70g
Copy link

@alber70g alber70g commented Nov 17, 2017

This fixes #827
When building from command line with --name <buildName>, it now takes the options from the command line, merges them with the project's options and continues the build.

It's probably fixing #613 as well.

  • CHANGELOG.md has been updated

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to see a regression test for this as well, if you are able to figure that out (not sure if we have existing testing infrastructure for that)

// If a config for the current buildName is present
// Merge the projects optimizeOptions with the options
// Commandline options will override the projects options
const projectOptions = polymerProject.config.builds.find(buildConfig => buildConfig.name === buildName);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can destructure css, js and html directly. Then line 49 instead of projectOptimizeOptions you can do {css, js, html}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what if one of the options is not available, then the option where it should be merged to will be overridden with undefined

Copy link
Author

@alber70g alber70g Nov 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, haven't seen any regression tests for the build/cli options. So that's why I didn't add one

@alber70g
Copy link
Author

alber70g commented Nov 17, 2017

Please take another look. I've changed it quite a bit. Should be better now

  • Command line options weren't taken into account previously.
  • When there are no command line options, all further options were overridden by 'undefined'

Can you tell me where to put the tests? Since it's quite a bit more complex than I thought at the start

[edit:] I've squashed all commits into one. It should be good now.

@alber70g
Copy link
Author

alber70g commented Nov 21, 2017

There are no tests for this at this moment. Please provide an infrastructure for it, or merge this MR because it fixes the --name <buildName> issue

@aomarks
Copy link
Member

aomarks commented Jan 3, 2019

Closing this pull request because this repo is now archived. If this PR is still active, please submit a new one at https://github.com/Polymer/tools.

@aomarks aomarks closed this Jan 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants