-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make explore experience default #10588
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
Make explore experience default #10588
Conversation
Signed-off-by: abbyhu2000 <abigailhu2000@gmail.com>
❌ Empty Changelog SectionThe Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10588 +/- ##
==========================================
- Coverage 60.25% 60.25% -0.01%
==========================================
Files 4385 4385
Lines 116753 116753
Branches 19010 19010
==========================================
- Hits 70346 70345 -1
Misses 41568 41568
- Partials 4839 4840 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Qingyang(Abby) Hu <abigailhu2000@gmail.com>
enabled: true, | ||
}, | ||
schema: configSchema, | ||
deprecations: ({ rename, unused }) => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abbyhu2000 Are deprecations the right way to do it. it seems from prior usage as a way to just annotate settings that are unused.
deprecations: ({ unused }) => [unused('defaultLanguage')], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea it was a bit of the customized usage of deprecations. But after investigation, i realized to auto-enable these settings when explore flag is on is to either use deprecations or to modify the core system code during plugin discovery phase(src/core/server/plugins/discovery/plugins_discovery.ts). I feel like changing the core plugin discovery code might be against the architecture, thus i utilize the deprecations as a hack. @ashwin-pc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets just call out this hack and mark it as not a pattern we want to follow in the comments. Given our start to build 4.0, this is just a stop gap solution with minimal changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this needs to block this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will merge this and add the comment in the following PR.
linked with #10490 |
Signed-off-by: Nathan Yang <yanatha@amazon.com>
Description
auto turn on the following config when explore is turned on
Issues Resolved
Screenshot
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration