-
Notifications
You must be signed in to change notification settings - Fork 28
[ContentFeature] de-decouple build specific code from content-features #1545
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
[ContentFeature] de-decouple build specific code from content-features #1545
Conversation
✅ Deploy Preview for content-scope-scripts ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Temporary Branch UpdateThe temporary branch has been updated with the latest changes. Below are the details:
Please use the above install command to update to the latest version. |
[Beta] Generated file diffTime updated: Thu, 13 Mar 2025 08:28:45 GMT Android
File has changed Chrome
File has changed Chrome-mv3
File has changed Firefox
File has changed Integration
File has changed Windows
File has changed Apple
File has changed |
f87255a
to
c5e86a4
Compare
894cce5
to
512b303
Compare
512b303
to
e7e6721
Compare
…uckduckgo/content-scope-scripts into dbajpeyi/refactor/decouple-import-meta
917f4f8
to
db3cdf8
Compare
injected/src/content-feature.js
Outdated
this.#args = null; | ||
constructor(featureName, importConfig) { | ||
super(featureName); | ||
this.args = null; |
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.
this.args = null; | |
this.#args = null; |
d309b19
to
85dfc98
Compare
85dfc98
to
363da49
Compare
83024d3
to
2562c00
Compare
6c388ce
to
0a3d39b
Compare
@jonathanKingston I've addressed the comments. Note that the references are PS: Integration test seems to be broken, but not sure why. I am investigating. |
@shakyShane this reshuffle starts simplifying some of the application structure. It breaks content-feature into two classes, one which autofill can then depend on or use in isolation. I suggest when Deep gets back we add more testing on this interface so we can add it as a testable interface rather than just letting this barrier to autofill to rot. This largely is a transitional step, I don't think we want to reside with args wiring still being so complex (i'd like to break the dependency of features heavily relying on them and instead use methods to access that data). This also simplifies the load and init arguments mostly as a requirement to remove the use of import.meta which I think we agree now was a bit of a mistake. We just then use it at the periphery and don't require autofill to need custom transpiling. importConfig is the transport object for anything that's import.meta related through into the features. I've reviewed most of this, I'd appreciate a quick look over in case I missed anything. The 4 additional small tiny commits I made just got the integration tests working from a missed setting of args in a method. |
@jonathanKingston I pushed a commit to fix a type, as mentioned in asana I tested this with complex integrations like duck.ai etc. |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Asana Task/Github Issue: https://app.asana.com/0/72649045549333/1209507178584805
Description
ConfigFeature
class, which allows outside packages to re-use functionality likegetFeatureSetting
, without having to implement the wholeContentFeature
import.meta
code from the core code, and pass it asimportConfig
in thecontent-scope-features.js
classTesting Steps
Checklist
Please tick all that apply: