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

WIP: Fix option namespace pollution #951

Closed

Conversation

shadowspawn
Copy link
Collaborator

@shadowspawn shadowspawn commented Apr 16, 2019

Work in progress. Added a feature flag to get new behaviour, where option values are not stored on the Command object but in a separate field and accessed using new accessor methods. The new behaviour is opt-in, unchanged clients get unchanged old behaviour for first release.

Builds on work in #515 and #673

Feedback welcome (from anyone). Still a work in progress, I am still coding to try ideas so anything could change. 🛠

I will move to branch on tj/commander if other maintainers want easier access to code.

Still under consideration:

  1. should new accessor methods be get+has (like Map) or getOption+hasOption for clarity? get/has for simplicity, like original properties
  2. better name for mapStyleOptions? storeOptionsAsProperties
  3. setFeatureFlag or setFeatureFlags? setFeatureFlags

Still to prototype:

  • special case get/has/setFeatureFlag options so they do not affect existing clients who do not opt-in
  • pass feature flag to commands so new behaviour is global
  • Typescript declarations for new routines
  • README
  • include existing .opts() in README description of options too
  • tests for new behaviour (and tests expecting old behaviour do not change)

Code fragment:

program
   .setFeatureFlags("storeOptionsAsProperties:false")
   .option("-o, once", "description");

if (program.has("once") console.log("Once upon a time");

Other changes:

  • took version out of new-behaviour option values, because it is not really an option value (?)
  • added warning for an old-behaviour option when can detect there is a conflict
  • used null object as prototype for new-behaviour options (so close to a pure map)

@shadowspawn shadowspawn changed the title WIP: Fix option name pollution WIP: Fix option namespace pollution Apr 16, 2019
@shadowspawn
Copy link
Collaborator Author

A side note: if we did not already have the somewhat undocumented .opts() routine I would have suggested having a property called .opts! Only two extra characters and arguable better separation to have a routine than a property, but I am used to process.argv et al.

- setFeatureFlags takes just dictionary object
- rename option handling feature flag
- fall through to old behaviour after warning
@shadowspawn
Copy link
Collaborator Author

shadowspawn commented Aug 16, 2019

My current thinking is:

  • a new map property is the canonical source of truth for option values. Name undecided.
  • do not add .get and .has to command, instead use map property
  • configuration to enable/disable legacy options-as-properties
  • keep existing .opts() to reduce breaking changes?

Considering:

  • configuration to control whether .action handler is passed Command, options object, or map

@shadowspawn
Copy link
Collaborator Author

I have some new ideas and going to start again, from latest code.

@shadowspawn shadowspawn closed this Nov 3, 2019
@shadowspawn shadowspawn deleted the feature/933-option-pollution branch January 11, 2020 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant