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

Remove window.loadExtensionView. #70

Merged
merged 1 commit into from
Jul 13, 2021
Merged

Conversation

dompuiu
Copy link
Member

@dompuiu dompuiu commented Jul 13, 2021

Description

Remove window.loadExtensionView from sandbox.

Motivation and Context

There are better ways to test the extension views.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dompuiu dompuiu requested a review from brenthosie July 13, 2021 16:25
Copy link
Member

@brenthosie brenthosie left a comment

Choose a reason for hiding this comment

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

What's the catalyst behind this change and will it unexpectedly introduce problems for customers relying on it?

@dompuiu dompuiu requested a review from Aaronius July 13, 2021 18:07
@dompuiu
Copy link
Member Author

dompuiu commented Jul 13, 2021

@Aaronius Would you be kind and review this change? It's related to the last email you sent to us where you said you no longer need it.

Copy link
Contributor

@Aaronius Aaronius left a comment

Choose a reason for hiding this comment

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

Looks good. To answer @brenthosie's question, I initially added this functionality because I thought it was a good way for extension developers to be able to test their extensions in an automated fashion using something like TestCafe and Cypress. We thought we needed this for the web SDK extension and we actually used it for a couple years, but recently devised a better approach that doesn't require us to use the sandbox by mocking the extension bridge coming from Akamai. It simplified code and sped up the tests as well, so I would advise anyone else to take a similar approach instead of using this loadExtensionView functionality.

I don't know if anyone else is using this functionality currently, but if they are, then this removal will force them to use a different approach. As such, this should be released as a major version.

@dompuiu dompuiu merged commit a688142 into master Jul 13, 2021
@dompuiu dompuiu deleted the remove_loadextensionview branch July 13, 2021 18:59
@Aaronius
Copy link
Contributor

For reference, here's what we ended up doing instead. When running functional tests, our extension requests the extension bridge from Akamai and we return a mock extension bridge file instead that allows our functional tests to call the extension's init, getSettings, and validate methods as though it were Launch. We're leveraging network mocking capabilities built into TestCafe, though other frameworks have similar functionality. Here's the mock extension bridge file we use in place of the real extension bridge. This allows us to not have to run the extension within reactor-sandbox or even an iframe when running functional tests, while still being able to test the init, getSettings, and validate logic of the extension. If any other extension developers run into the same need, you could point them to this approach.

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