Skip to content

Add global export variable #18

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

Merged
merged 12 commits into from
Nov 14, 2016
Merged

Add global export variable #18

merged 12 commits into from
Nov 14, 2016

Conversation

mikeproeng37
Copy link
Contributor

@mikeproeng37 mikeproeng37 commented Nov 14, 2016

@optimizely/fullstack-devs

Fixes #14

@mikeproeng37 mikeproeng37 mentioned this pull request Nov 14, 2016
"main": "index.js",
"scripts": {
"test": "./node_modules/.bin/mocha ./tests.js",
"build": "webpack index.js dist/optimizely.min.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest changing this to: "build": "webpack $npm_package_main dist/optimizely.min.js", so as to reduce the duplicate knowledge of the entrypoint. This way main is the canonical location for defining the entrypoint and the build script doesn't need tweaked if the entrypoint is moved.

@jasonkarns
Copy link
Contributor

Seems that with the removal of the json-loader from the webpack config, it can be removed as a devDep, as well.

@mikeproeng37
Copy link
Contributor Author

Thanks for the suggestions @jasonkarns . Will address those in a PR into the devel branch as it would just delay this PR.

@mikeproeng37 mikeproeng37 merged commit f8bed21 into master Nov 14, 2016
mjc1283 added a commit that referenced this pull request Apr 22, 2019
Summary:

Before this change, we were passing the host property returned from url.parse as the host option going into http.request (or https.request). The host property from url.parse includes the port when the URL contains a port.
The problem is, request needs port to be passed as a separate option (not included as part of the host option).
To fix, stop passing host, and replace it with hostname: url.hostname (where url.hostname excludes port), and port: url.port.

References:

https://nodejs.org/api/http.html#http_http_request_url_options_callback
https://nodejs.org/api/url.html#url_urlobject_host
https://nodejs.org/api/url.html#url_urlobject_hostname
https://nodejs.org/api/url.html#url_urlobject_port

Test plan:

Added a regression test. Manually tested in @mikeng13's prototype datafile management branches for JS-testapp and compatibility suite.
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