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.

Sign up for GitHub

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

Jump to bottom

Using a config to include ember-data/debug in the build #6599

Merged
merged 1 commit into from
Nov 12, 2019
Merged

Using a config to include ember-data/debug in the build #6599

merged 1 commit into from
Nov 12, 2019

Conversation

pete-the-pete
Copy link
Contributor

Part of #6166 , the debug package is included based on a config setting in environment.js.

This relies on the isEnabled hook of the debug package's index.js. I manually ran yarn build in the following scenarios with the following outcomes:

  1. in debug package -> not included
  2. in the main addon -> included
  3. in the main added with ENV.includeDebug = false; in environment.js -> not included

I used the environment.js settings to hold the potential config value, but this is mostly a POC, please let me know if there is a better location, name, etc.

@pete-the-pete
Copy link
Contributor Author

@runspired first pass.

runspired
runspired requested changes Oct 16, 2019
View reviewed changes
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

This is a great start! I think we want the config to source itself a bit differently, left an example inline.

packages/debug/index.js Outdated Show resolved Hide resolved
packages/debug/index.js Outdated Show resolved Hide resolved
@runspired runspired added the 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166 label Nov 5, 2019
@pete-the-pete pete-the-pete force-pushed the include-debug-by-config branch from bd98d69 to 636ff49 Compare November 5, 2019 20:37
@runspired runspired force-pushed the include-debug-by-config branch 2 times, most recently from e9ebff7 to fc8ef13 Compare November 12, 2019 20:06
@runspired runspired self-requested a review November 12, 2019 20:09
runspired
runspired approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Confirmed this works locally by testing it with the fastboot-test-app and adapter-encapsulation-test-app, the former consumes ember-data and the latter consumed @ember-data/debug directly.

@runspired runspired added 🏷️ feat This PR introduces a new feature 🎯 canary PR is targeting canary (default) labels Nov 12, 2019
@runspired runspired force-pushed the include-debug-by-config branch from fc8ef13 to 25148e4 Compare November 12, 2019 20:28
@runspired
Using a config to include ember-data/debug in the build
15ba731 
Updates with @runspired...and lots of console.log to see what's going on
Polish off

Co-authored-by: Chris Thoburn <runspired@gmail.com>
@runspired runspired force-pushed the include-debug-by-config branch from 25148e4 to 15ba731 Compare November 12, 2019 21:23
@runspired runspired merged commit 712bcf1 into emberjs:master Nov 12, 2019
@runspired runspired removed the 🎯 canary PR is targeting canary (default) label May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@runspired runspired runspired approved these changes

Assignees
No one assigned
Labels
🏷️ feat This PR introduces a new feature 🌲 Project Trim 🌲 PRs related to https://github.com/emberjs/data/issues/6166
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pete-the-pete @runspired