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

Support selection of Chrome DevTools protocol version (and fixes current version which is incomplete) #27

Merged
merged 2 commits into from
May 2, 2019

Conversation

holsee
Copy link
Contributor

@holsee holsee commented Apr 30, 2019

  • Adds support for selecting different versions of the Chrome DevTools protocol through environment variables, defaulting to version "1-3".
  • Adds all protocols "1-2", "1-3" and "tot" to ./priv.
  • Fixes issue with ./priv/protocol.json whereby it was missing many of the domains which are contained within "1-3".

@holsee holsee changed the title Feature/protocol flags Support selection of Chrome DevTools protocol version (and fixes current version which is incomplete) Apr 30, 2019
@andrewvy
Copy link
Owner

I'm a bit iffy about introducing a CRI_PROTOCOL_VERSION environment variable for a compile-flag, can we use Mix config and read it at compile-time?

A quick fix, I should make a mix task to fetch these from https://github.com/ChromeDevTools/debugger-protocol-viewer/tree/master/_data as a quick standardization.

@holsee
Copy link
Contributor Author

holsee commented Apr 30, 2019

Regarding the refresh of the protocol files I would recommend that discussion be moved to another issue.

Regarding compile time configuration:

  • using config.ex is seen as an anti-pattern in libraries (see elixir 1.9 where it won’t be generated by default anymore on mix new)
  • app config is read and populated for access at runtime, and as such won’t be suitable for compile time access
  • environment variables are commonly used for configuring this kind of thing and work well in many scenarios including creating docker images.

@andrewvy
Copy link
Owner

andrewvy commented May 2, 2019

Hmm, I'm still iffy on the changes, it's odd to me since Logger configuration also provides compile-time flags to purge log levels from code.

If you insist, I merged it in, we can always change this in the future.

@holsee
Copy link
Contributor Author

holsee commented May 2, 2019

I agree we can look at this again. Maybe my understanding is flawed, but app configuration is certainly a runtime thing, maybe it is accessible from elixir compiler which hydrates it 🤔, but in the meantime, the default behaviour remains unchanged and the option is there for those who wish to alter the protocol compile target.

Thanks!

@holsee holsee deleted the feature/protocol-flags branch May 2, 2019 18:04
@andrewvy
Copy link
Owner

andrewvy commented May 2, 2019

Definitely! I think it's odd that application configuration is available at both compile-time + run-time, though I agree the primary usage is 99% runtime-based configuration. I've never had the chance to deal with compile-time flags in an Elixir lib before too. 😄

Thanks for the PR!

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.

2 participants