Skip to content

revert "Allow instrumentation to be turned on using .ember-cl"#7738

Closed
GavinJoyce wants to merge 1 commit intoember-cli:masterfrom
GavinJoyce:gj/revert-patoc/config-instrumentation
Closed

revert "Allow instrumentation to be turned on using .ember-cl"#7738
GavinJoyce wants to merge 1 commit intoember-cli:masterfrom
GavinJoyce:gj/revert-patoc/config-instrumentation

Conversation

@GavinJoyce
Copy link
Copy Markdown
Contributor

@GavinJoyce GavinJoyce commented Apr 5, 2018

Reverts #7491

This feature resulted in a bunch of blueprint test failures for tests in ember-source, see this discussion.

Reverting this will unblock some MU blueprint work and give us some time to rethink the implementation of this feature.

return configOverride;
}

if (config === undefined) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of revertting the whole feature, I think we can just remove the caching here. Without this caching, we would effectively have the same behavior as before WRT blueprint testing, and in the normal ember serve case we would only call this method once anyways...

What do you think?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to remove caching, I was being conservative as I'm not familiar with the impact of removing it. I'll prepare a separate PR while we consider

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This function essentially becomes:

function getConfig(override) {
  if (override) { return override; }

  return generateConfig();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sounds good. FWIW, this caching should only affect the blueprint testing case. In "normal" command line usage, we would only ever call getConfig once anyways...

Copy link
Copy Markdown
Contributor Author

@GavinJoyce GavinJoyce Apr 5, 2018

Choose a reason for hiding this comment

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

It's also called in instrumentationEnabled. I'm not currently familiar with where or how often that's used though

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for sorting @GavinJoyce 👏

@rwjblue I was under the impression that it would have been called quite frequently in the instrumentation checks but I could be wrong. Is it just called on instrumentation set up?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ya, its called 3 times

@GavinJoyce
Copy link
Copy Markdown
Contributor Author

closing in favour of #7739

@GavinJoyce GavinJoyce closed this Apr 5, 2018
@GavinJoyce GavinJoyce deleted the gj/revert-patoc/config-instrumentation branch April 5, 2018 19:04
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.

3 participants