Skip to content

fix(build): Resume UMD bundle build, and restore docs #152

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
Merged
merged 6 commits into from
Aug 20, 2018
Merged

fix(build): Resume UMD bundle build, and restore docs #152

merged 6 commits into from
Aug 20, 2018

Conversation

spencerwilson-optimizely
Copy link
Contributor

@spencerwilson-optimizely spencerwilson-optimizely commented Aug 20, 2018 •
edited
Loading

Summary

  • Restore the build-browser-umd npm script, which produces dist/optimizely.browser.umd.min.js
  • Change the UMD lib name optimizelyClient -> optimizelySdk, since the createInstance client factory function may be one of several exports of the SDK, in the future.
  • (not a visible change) Change Markdown indentation of some bull points, to please my Markdown editor Typora

This undoes one bit of #135. I hadn't realized that this asset was documented, and does have some uses, like faster/easier prototyping in a browser.

Test plan

I made a foo.html with

<html>
<head>
  <script src="./dist/optimizely.browser.umd.min.js"></script>
  <script>
    console.log('trying sdk');
    window.optimizelySdk
    console.log('trying client');
    window.optimizelyClient
    console.log('trying client');
    window.optimizelyClient
  </script>
</head>
</html>

and it still works:

screen shot 2018-08-20 at 2 59 47 pm

@coveralls
Copy link

coveralls commented Aug 20, 2018 •
edited
Loading

Coverage Status

Coverage decreased (-0.06%) to 97.051% when pulling 473ca41 on sw/reintroduce-build-step into 97b4aa2 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.11% when pulling 5e7c423 on sw/reintroduce-build-step into 97b4aa2 on master.

tylerbrandt
tylerbrandt reviewed Aug 20, 2018
View reviewed changes
Copy link
Contributor

@tylerbrandt tylerbrandt left a comment

Choose a reason for hiding this comment

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

Can we arbitrarily change the window reference? We do have docs claiming window.optimizelyClient exists. at least I think we'd have to define it as an alias of window.optimizelySdk.

@spencerwilson-optimizely
Copy link
Contributor Author

The current docs say

If you aren't able to install via npm or to use the SDK in a non-CommonJS fashion, follow the instructions in the SDK's README to build a minified snippet. You can then access the SDK from window.optimizelyClient. Note that this exposes the SDK as a whole, and you can instantiate a specific client using window.optimizelyClient.

...which IMO is pretty confusing, but yes they do duplicate what was previously (and with this PR, again) mentioned in the package readme. I suppose we could keep it until if/when the package has exports from its entry point other than createInstance. I'm not a huge fan of the alias idea since while it makes backwards compat totally smooth, autocomplete in browser JS consoles will show both optimizelyClient and optimizelySdk, which may cause confusion.

@mikeng13 what do you think?

@mikeproeng37
Copy link
Contributor

Yeah we do have devs out there who are using optimizelyClient. I am fine with breaking support for it now in favor of optimizelySdk. Just make sure to update the old docs (dev docs) and the new docs at READme

@mikeproeng37
Copy link
Contributor

build

mikeproeng37
mikeproeng37 approved these changes Aug 20, 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

@spencerwilson-optimizely
Copy link
Contributor Author

Okay, that's good few users are affected. However, I think it's valuable to maintain backwards compatibility. What do yall think of

  1. Aliasing, per @tylerbrandt 's suggestion
  2. Logging a warning on first access to the alias

See the updated test plan in the PR description for an example of how this works.

@spencerwilson-optimizely spencerwilson-optimizely merged commit b1d6891 into master Aug 20, 2018
@spencerwilson-optimizely spencerwilson-optimizely deleted the sw/reintroduce-build-step branch August 20, 2018 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tylerbrandt tylerbrandt tylerbrandt approved these changes

@mikeproeng37 mikeproeng37 mikeproeng37 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
@spencerwilson-optimizely @coveralls @mikeproeng37 @tylerbrandt