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

@W-16797436@ [App Extensibility ⚙️] Allow overrides to be disabled #2051

Merged

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Oct 4, 2024

Description

In this PR I ensure that any overrides that exist in a project DO NOT get used in the module resolution and building of the bundled code if they are DISABLED. This is mirror behaviour of how we handle the Application Extension initialization.

There is 1 caveat however, and that is that you'll most likely need to restart your dev server to see the effects of disabling extensions specifically with overrides as the module resolution logic doesn't re-run. We'll make sure we document that in a known behaviours in a quip doc.

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Changes

  • Filter out disabled application extensions when generating lists of possible modules paths in buildCandidatePaths
  • Added example extension that will be removed before merging this PR.

How to Test-Drive This PR

  • Check out this code.
  • Run the typescript minimal project and notice that there is a red border around the application.
  • Now edit the package.json to "enable" the "@salesforce/extension-sample-with-overrides" extension by setting enabled to true.
  • Restart the dev server.
  • Notice that the border is now green because an override in that extension is now running.

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

@bendvc bendvc requested a review from a team as a code owner October 4, 2024 18:43
@bendvc bendvc requested review from vmarta and adamraya October 4, 2024 20:32
Comment on lines +105 to +107
paths = expand(extensionEntries)
.filter(isEnabled)
.reverse()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you consider extracting the filtering logic for enabled extensions into a separate function to simplify buildCandidatePaths and perhaps include a test?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think a single line to filter doesn't really warrant being refactored, unless I'm just not understanding how that would be done. Also, I did add some tests that specifically pass extension entries that are disabled to ensure they are getting filtered out. Also the isEnabled function is tested too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I assumed the logic was in the same function, but reading the code further, I see isEnabled is already a separate function with its own tests.

const srcPath = path.join(projectDir, NODE_MODULES, extensionRef, SRC)
return [...acc, path.join(srcPath, OVERRIDES, importPath), path.join(srcPath, importPath)]
}, [])
paths = expand(extensionEntries)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we validate the paths exsist before processing them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. This already happen internally in the resolve call so we can just lean on it to do the right thing.

@@ -1,4 +1,5 @@
## v4.0.0-dev (Jun 21, 2024)
- Overrides in `disabled` Application Extensions will take no effect. [#2051](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2051)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Overrides in `disabled` Application Extensions will take no effect. [#2051](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2051)
- Overrides in `enabled: false` Application Extensions will take no effect. [#2051](https://github.com/SalesforceCommerceCloud/pwa-kit/pull/2051)

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not sure if you want to call out the actual property name enabled

@bendvc bendvc merged commit 5c1f13b into feature/extensibility-v2 Oct 4, 2024
17 of 29 checks passed
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