Njb/use cross env #105
Njb/use cross env #105
Conversation
|
Feathers uses shx ( https://github.com/shelljs/shx ) and I don't want to introduce another player. Would you mind converting to shx? |
|
@eddyystop sounds good. Looking into it briefly I'm not sure how I would do this with shx though. I found this issue which references cross-env and it sounds like it's not really possible in shx. Do you have any ideas? |
|
@daffl ^^^ |
|
Is there any way at all to continue using shx? Otherwise
|
|
I have little experience with shx, but from what's said in shelljs/shx#127 it seems like it can't replace |
|
@NickBolles Could you confirm that you can't do what you want to with shx. Shoujld that be the case, could you resolve the merge conflicts and I will merge. |
|
Yea, I couldn't get it to work. I'll resolve right now then it should be ready to merge. |
| let env = (config.tests || {}).environmentsAllowingSeedData || []<%- sc %> | ||
| let ifDbChangesAllowed = env.includes(process.env.NODE_ENV)<%- sc %> | ||
| let { tests: { environmentsAllowingSeedData = [] } = {}} = config<%- sc %> | ||
| let ifDbChangesAllowed = environmentsAllowingSeedData.includes(process.env.NODE_ENV)<%- sc %> |
There was a problem hiding this comment.
Could you revert these changes, too?
eddyystop [2:11 PM]
I've started using (config.tests || {}).environmentsAllowingSeedData because it can later be easily replaced by the optional chaining proposal config?.tests?.environmentsAllowingSeedData https://dev.to/sammyisa/optional-chaining-may-be-coming-to-javascript--4ff0
There was a problem hiding this comment.
I changed this to be a little utility function that takes the config. This way we can pass the config from the app instance
There was a problem hiding this comment.
The thing I liked about ifDbChangesAllowed was that the dev can refer to it in the custom code. Its simpler than having to call the function with app.get('tests'). That var name can be a standard throughout DB mutating tests.
I don't want to cause a fuss but the generated code should always be as flexible as possible for customizations.
There was a problem hiding this comment.
I can undo this. But I don't really see how it makes things less flexible and it seems like importing the json directly will break config and introduce bugs.
I definitely get what you are getting at though, we want the code to be as flexible as possible, but this seems like a case where there's benefit to making this change, and that there's really one use to the variable, short circuit the seed data function
There was a problem hiding this comment.
I agree the import should be replaced by app.get.
I was probably not clear enough. I like having ifDbChangesAllowed as an already set value for the user as it easier to find than the function.
It's possible the user remove the generated exit for something else and having a set value looks advantageous.
There was a problem hiding this comment.
@eddyystop ahhh I see what you mean. Check my most recent commit for an update on this. Let me know if you were thinking anything different.
|
@j2L4e I updated those changes. Thanks for looking at them! |
|
Completed this PR with #137 |
|
Thank for the help you guys! |
Summary
If so, please mention them to keep the conversations linked together.
Other Information
Use cross-env to set environment variables so that the scripts don't bomb out on windows