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

Add strict-mode proxy wrapper #44

Merged
merged 5 commits into from
Aug 14, 2017
Merged

Add strict-mode proxy wrapper #44

merged 5 commits into from
Aug 14, 2017

Conversation

af
Copy link
Owner

@af af commented Aug 14, 2017

Fixes #43. @SimenB could you double check this? You have push access now so feel free to merge it too if it looks good :)

// if (name === 'nodeType') return undefined

const val = envObj[name]
if (val === undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use hasOwnProperty instead?


const val = envObj[name]
if (val === undefined) {
throw new Error(`Environment var not found: ${name}`)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add [envalid] prefix here as well, to be consistent

@af
Copy link
Owner Author

af commented Aug 14, 2017

@SimenB Great feedback, thanks! Last commit should address both points

Copy link
Collaborator

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

LGTM!

// if (name === 'nodeType') return undefined

const varExists = envObj.hasOwnProperty(name)
if (!varExists) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

varExists can be inlined

const varExists = envObj.hasOwnProperty(name)
if (!varExists) {
throw new Error(`[envalid] Environment var not found: ${name}`)
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unneeded else

Copy link
Owner Author

Choose a reason for hiding this comment

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

oh yeah, good point :)

@af
Copy link
Owner Author

af commented Aug 14, 2017

Thanks!

@af af merged commit 4680535 into master Aug 14, 2017
@af af deleted the strict-proxy branch August 14, 2017 16:10
@SimenB
Copy link
Collaborator

SimenB commented Aug 18, 2017

@af could you do a publish? (I guess it's a major unless it's moved away from strict to something like reallyStrict)

@af
Copy link
Owner Author

af commented Aug 18, 2017

@SimenB This is a breaking change so next release will be 4.0. I'd really like to resolve #37 and possibly #45 before shipping that; could you pin to a git version in the meantime?

@SimenB
Copy link
Collaborator

SimenB commented Aug 26, 2017

@af now that those are closed, anything else you want to get done before release?

@af
Copy link
Owner Author

af commented Aug 26, 2017

@SimenB just your prettier PR now I guess :)

@SimenB
Copy link
Collaborator

SimenB commented Aug 26, 2017

Woo

@af
Copy link
Owner Author

af commented Aug 26, 2017

@SimenB 4.0.0 is live! Thanks for all your help :)

@SimenB
Copy link
Collaborator

SimenB commented Aug 26, 2017

Awesome! Looking forward to Monday and deleting some code 🙂

@SimenB
Copy link
Collaborator

SimenB commented Aug 28, 2017

image

🎉

@af
Copy link
Owner Author

af commented Aug 28, 2017

\o/

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.

2 participants