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

refactor(dist): update bundled files #189

Merged
merged 7 commits into from
Nov 15, 2018
Merged

refactor(dist): update bundled files #189

merged 7 commits into from
Nov 15, 2018

Conversation

tylerbrandt
Copy link
Contributor

@tylerbrandt tylerbrandt commented Nov 14, 2018 •
edited
Loading

Summary

BREAKING: Drop support for window.optimizelyClient, as presaged in the CHANGELOG.

  • Pull in changes from 2.3.x, to ensure that both optimizely.browser.umd.js and optimizely.browser.umd.min.js are defined.
  • Refactor those changes to use webpack config/index.browser code instead of appending to the bundle after-the-fact.
  • Actually install webpack as a devDependency rather than always using latest

In addition to fixing the issue from #187 in this branch, it simplifies the build script.

Test plan

Existing unit tests pass since the "node" API for index.browser.js wasn't changed.

Created a test file called "dist/test.html" and loaded it in a browser (as well as an equivalent one that loads the min.js variant):

<!DOCTYPE html>
<html>

<head>
	<script src="/optimizely.browser.umd.js"></script>
</head>

<body>
	<h1>Hello</h1>
	<script>
		console.log('optimizelySdk', typeof optimizelySdk);
		console.log('optimizelyClient', typeof optimizelyClient);
	</script>
</body>

</html>

Ensured that it logged "object" for the first, and "undefined" for the second:
screen shot 2018-11-14 at 3 50 30 pm

Issues

  • OASIS-3790

@tylerbrandt tylerbrandt changed the title Tyler/merge 2.3 refactor(dist): update bundled files Nov 14, 2018
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.141% when pulling 13be72d on tyler/merge-2.3 into 2a47e5e on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.141% when pulling 13be72d on tyler/merge-2.3 into 2a47e5e on master.

@coveralls
Copy link

coveralls commented Nov 14, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 97.328% when pulling e829c3b on tyler/merge-2.3 into 2a47e5e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.141% when pulling 13be72d on tyler/merge-2.3 into 2a47e5e on master.

@mikeproeng37
Copy link
Contributor

This needs to be accompanied by a change to our public docs when we release. Can you include the an entry for a breaking change under unreleased for the deprecated optimizelyClient reference?

@tylerbrandt
Copy link
Contributor Author

This needs to be accompanied by a change to our public docs when we release. Can you include the an entry for a breaking change under unreleased for the deprecated optimizelyClient reference?

Why? The docs already only refer to window.optimizelySdk and we published a changelog entry in August that said that window.optimizelyClient is going away.

mikeproeng37
mikeproeng37 approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm

@tylerbrandt
Copy link
Contributor Author

Any comment on this?

perhaps they should be modified to expect window.optimizelySdk; comment if you agree/disagree

@mikeproeng37
Copy link
Contributor

Any comment on this?

perhaps they should be modified to expect window.optimizelySdk; comment if you agree/disagree

I agree we should be doing this check in our tests.

@tylerbrandt
Copy link
Contributor Author

Hmm I can't figure out how to set it up with webpack/karma. Might have to be an integration-type test. Added #190

spencerwilson-optimizely
spencerwilson-optimizely approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@spencerwilson-optimizely spencerwilson-optimizely left a comment

Choose a reason for hiding this comment

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

Cool, thanks for simplifying

@mikeproeng37
Copy link
Contributor

Hmm I can't figure out how to set it up with webpack/karma. Might have to be an integration-type test. Added #190

Alright we'll take care of it in another PR then.

@tylerbrandt tylerbrandt merged commit 6f0b6a4 into master Nov 15, 2018
@tylerbrandt tylerbrandt deleted the tyler/merge-2.3 branch November 15, 2018 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeproeng37 mikeproeng37 mikeproeng37 approved these changes

@spencerwilson-optimizely spencerwilson-optimizely spencerwilson-optimizely approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tylerbrandt @coveralls @mikeproeng37 @spencerwilson-optimizely