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

Wrong options type when set from environment variables #118

Open
danielsanfr opened this issue Nov 17, 2020 · 4 comments
Open

Wrong options type when set from environment variables #118

danielsanfr opened this issue Nov 17, 2020 · 4 comments

Comments

@danielsanfr
Copy link
Contributor

Hello.

While I was implementing PR: #117, I encountered a problem with the environment variables.

As you can see in the image below, the _baseUrlDirect variable has the correct type (boolean), as it was not set via the environment variable. However, the other two _directAccess and _presignedUrl variables that were set as environment variables, are not being converted to the correct type (it should be boolean).

2020-11-17_00-45

This problem causes a check of the form if (_directAccess) { ... }, do not be verified as expected.

I would like to know if I can create a PR using the https://www.npmjs.com/package/boolean lib to do this conversion?

I don't know if you noticed, but the _presignedUrlExpires variable should be an integer, but it is also a string. However, this case can be solved with the standard parseInt() Javascript library.

@davimacedo
Copy link
Member

Thanks for reporting. I believe we do not need a package for that. We just need to convert in this line: https://github.com/parse-community/parse-server-s3-adapter/blob/master/lib/optionsFromArguments.js#L91

Do you want to open a PR?

@danielsanfr
Copy link
Contributor Author

OK, sorry if I'm being boring, but I will need this solution for 3 variables baseUrlRirect, directAccess and presignedUrl. I will end up having to create a function for this. But looking at the library code, it only has 1 function, has no dependency and already has unit tests. I don't see why not use it?

https://github.com/thenativeweb/boolean/blob/master/lib/boolean.ts

It has nothing to do with this issue, but I don't know if I should create another issue to remove this doubt.
Why does this project depend on the parse SDK if it is not used for anything? I think we could remove that dependency.

@davimacedo
Copy link
Member

Ok. No problem. You can use the library.

@danielsanfr
Copy link
Contributor Author

Thanks! I will do a PR as soon as the PR (#117) has been accepted.

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

No branches or pull requests

2 participants