8000 feat: Convert config_validator to TS by yavorona · Pull Request #556 · optimizely/javascript-sdk · GitHub
[go: up one dir, main page]
Skip to content

feat: Convert config_validator to TS #556

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

Merged
merged 5 commits into from
Sep 2, 2020
Merged

feat: Convert config_validator to TS #556

merged 5 commits into from
Sep 2, 2020

Conversation

yavorona
Copy link
Contributor

Summary

  • Convert config_validator module from JS to TS.

Test plan

  • Existing unit tests

@yavorona yavorona requested a review from a team as a code owner August 20, 2020 16:51
@yavorona yavorona self-assigned this Aug 20, 2020
@yavorona yavorona changed the title Convert config_validator to TS feat: Convert config_validator to TS Aug 20, 2020
@coveralls
Copy link
coveralls commented Aug 20, 2020 •
edited
Loading

Coverage Status

Coverage decreased (-0.03%) to 96.741% when pulling a462f65 on pnguen/convert-config-validator-to-ts into d8d9f71 on master.

mjc1283
mjc1283 approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor
@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

LGTM. One non-blocking comment about naming of validate function.

packages/optimizely-sdk/lib/index.browser.js Outdated
@@ -73,7 +73,7 @@ var createInstance = function(config) {
}

try {
configValidator.validate(config);
validate(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It is a little hard to understand what validate is - IMO it would be easier to read as import * as configValidator from '...'; configValidator.validate(config);

packages/optimizely-sdk/lib/index.node.js Outdated
@@ -70,7 +70,7 @@ var createInstance = function(config) {
}

try {
configValidator.validate(config);
validate(config);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment re: validate vs. configValidator.validate

@mjc1283
Copy link
Contributor
mjc1283 commented Aug 20, 2020

@yavorona Please take a look at the FSC failure, I'm not sure what happened there.

@yavorona yavorona removed their assignment Aug 27, 2020
@yavorona yavorona force-pushed the pnguen/convert-config-validator-to-ts branch 5 times, most recently from 8c03d05 to c9a1366 Compare August 27, 2020 17:30
@yavorona yavorona requested a review from mjc1283 August 27, 2020 18:03
@yavorona yavorona force-pushed the pnguen/convert-config-validator-to-ts branch from c9a1366 to 9ddcb85 Compare August 31, 2020 19:49
@yavorona yavorona closed this Sep 1, 2020
@yavorona yavorona reopened this Sep 1, 2020
@yavorona yavorona removed their assignment Sep 1, 2020
@yavorona yavorona force-pushed the pnguen/convert-config-validator-to-ts branch from 9ddcb85 to a462f65 Compare September 2, 2020 00:28
@yavorona yavorona merged commit 80b4108 into master Sep 2, 2020
@yavorona yavorona deleted the pnguen/convert-config-validator-to-ts branch September 2, 2020 02:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjc1283 mjc1283 mjc1283 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.
3 participants
@yavorona @coveralls @mjc1283
0