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

[#71] Implement custom accessors #72

Merged
merged 18 commits into from
Sep 5, 2019
Merged

[#71] Implement custom accessors #72

merged 18 commits into from
Sep 5, 2019

Conversation

todofixthis
Copy link
Contributor

@todofixthis todofixthis commented Aug 27, 2019

Note: depends on #75

closes #71

  • Adds an optional extraAccessors param to from() that allows attaching a custom accessor to that instance. Any variables subsequently gotten from that instance will have those custom accessors available.
    • Each instance has its own set of custom accessors.
    • It is allowed to override custom or even built-in accessors.

Example:

± % BOOTNODES="127.0.0.1, 192.168.1.1:8000" node
> const { from } = require('.')

> const env = from(process.env, {
... asNodes: (raiseError, value, type = Set) => {
.....   return new type(
.......     value.split(',')
.......       .map(s => s.trim())
.......       .map(s => s.split(':', 2))
.......       .map(([ip, port = 80]) => ({ ip, port }))
.......   )
..... }
... })

> const bootnodes = env.get('BOOTNODES').asNodes()

> bootnodes
Set {
  { ip: '127.0.0.1', port: 80 },
  { ip: '192.168.1.1', port: '8000' } }

See unit tests for additional examples.

@todofixthis todofixthis changed the title 71 custom accessors [#71] Implement custom accessors Aug 27, 2019
@todofixthis
Copy link
Contributor Author

todofixthis commented Aug 27, 2019

Review requested:

  • I'm completely new to TypeScript, so I wasn't sure what (if any) changes to make to env-var.d.ts and test/index.ts.
  • Code coverage check fails, but I'm not familiar enough with the toolset to interpret the output.

Copy link
Owner

@evanshortiss evanshortiss left a comment

Choose a reason for hiding this comment

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

Nice PR, just minor changes and it's good to go.

lib/variable.js Outdated Show resolved Hide resolved
env-var.js Outdated Show resolved Hide resolved
@evanshortiss
Copy link
Owner

@todofixthis the PR fails on Node.js 6 since Object.entries isn't available. This ok though since 6 is no longer LTS. Can you delete these lines from travis.yml in your PR?

The other issue is code coverage dropped

@evanshortiss
Copy link
Owner

Don't worry about the TS stuff, I'm happy to look at it if you're not familiar 😃

@todofixthis
Copy link
Contributor Author

Ah, I should also add requisite documentation to the README while I'm at it 😅

Will tackle these items tomorrow 😺

@evanshortiss
Copy link
Owner

Oh right, I forgot README. Blaming that on my jet lag...

Thanks!

- Drop support for Node 6 & 7 (no longer LTS).
- Address some issues causing code coverage drop.
@coveralls
Copy link

coveralls commented Aug 27, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling adb308e on todofixthis:71-custom-accessors into be95f31 on evanshortiss:master.

@todofixthis
Copy link
Contributor Author

@evanshortiss All changes have been applied to this PR. Just need some help with the TypeScript stuffs, and then this is ready for final review 😺

@evanshortiss
Copy link
Owner

evanshortiss commented Aug 30, 2019

@todofixthis I've been trying to work out some TS types for this, but it's a tricky one since it mutates the underlying env instance. Mutations usually aren't TypeScript friendly, and it's being super unfriendly here. Obviously not ideal news, given your great work so far. What do you think of the changes described below? 😐

A solution I could get working with TypeScript looks similar to this in JS, since it returns a new env instance instead of mutating one. It also needs to directly throw EnvVarError instead of raiseError:

const { from, EnvVarError } = require('env-var')

const env = from(process.env, {
  asEmailComponents: (value) => {
	const parts = value.split('@')

	if (parts.length != 2) {
	  throw new EnvVarError('probably not an email')
	} else {
	  return {
        username: parts[0],
        domain: parts[1]
      }
    }
  }
})

const emailParts = env.get('EMAIL').asEmailComponents()

We don't have to use from. Maybe renaming it create makes sense, but I think this gets the idea across.

Here's what it looks like in TypeScript:

import { from, AccessorFn, EnvVarError } from 'env-var'

interface EmailParts {
  username: string
  domain: string
}

// EmailParts is the desired return structure/type
// AccessorFn is the type developers must use to define an accessor
const asEmailComponents: AccessorFn<EmailParts> = (value) => {
  const parts = value.split('@')

  if (parts.length != 2) {
    throw new EnvVarError('maybe not an email')
  } else {
    return {
      username: parts[0],
      domain: parts[1]
    }
  }
}

const env = from(process.env, {
  asEmailComponents
})

const emailParts = env.get('EMAIL').asEmailComponents()

This complicates things a bit implementation-wise since generateAccessors needs to pass raiseError to built-in accessors, but not pass it to the added accessors.

@todofixthis
Copy link
Contributor Author

todofixthis commented Sep 1, 2019

Interesting. Actually, I was going to ask about raiseError — wondering if it might be possible to have generateAccessor() catch exceptions raised by the accessor and handle them accordingly, so that we don't need raiseError at all.

I'll see if I can whip up an implementation for that, which might make this one easier to merge.

If an accessor cannot process an input value, it should just throw an exception instead.

There is a trade-off to this approach, in that the stacktrace for the accessor-thrown error is not preserved (replaced by `_raiseError()`), but accessors tend to be fairly simple in implementation, so it shouldn' be too difficult to locate the source of an error.
@todofixthis
Copy link
Contributor Author

Created #75 to replace raiseError with explicitly throwing errors. Let me know if that looks good, and if so I'll merge that into this one.

@todofixthis
Copy link
Contributor Author

I will also modify the code so that adding extra accessors happens at initialisation time 😺

@evanshortiss
Copy link
Owner

@todofixthis sounds good to me. That other PR is good to go, I just have one question on it before merging 👍

@todofixthis
Copy link
Contributor Author

Woohoo! I'll make the requested change tomorrow 😺

@todofixthis
Copy link
Contributor Author

#75 is good to go 😺 I've gone ahead and merged that branch into this one as well, to make sure that we can merge both of them into master cleanly.

@evanshortiss
Copy link
Owner

@todofixthis awesome work, and thanks for your patience with the TS compatibility bits 👍

I'll merge this, sort out the types, and release as 5.1.0 ASAP.

@evanshortiss evanshortiss merged commit a95ac7b into evanshortiss:master Sep 5, 2019
@todofixthis todofixthis deleted the 71-custom-accessors branch September 5, 2019 22:39
@todofixthis todofixthis restored the 71-custom-accessors branch September 9, 2019 21:26
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.

Feature: ability to add custom accessors
3 participants