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

Abstract Production vs Development #429

Merged
merged 1 commit into from
Nov 18, 2014

Conversation

Martii
Copy link
Member

@Martii Martii commented Nov 18, 2014

  • This is pre-approved by sizzle ... insert into every project .js file
  • Redirect existing conditionals to use these properties
  • The debug library may be improved separately in another issue but needs to and should remain the same property usage
  • Manual fix some of the missing/extra semi-colons found thanks to @jerone since in here ... this is probably incomplete and needs another go around
  • Manual fix some styleguide conformance found by @jerone with missing space
  • Remove one of my stale line notes

Tested on dev okay

Closes #428

* This is pre-approved by sizzle ... insert into every project .js file
* Redirect existing conditionals to use these properties
* The debug library may be improved separately in another issue but needs to and should remain the same property usage
* Manual fix some of the missing/extra semi-colons found thanks to @jerone since in here ... this is probably incomplete and needs another go around
* Manual fix **some** styleguide conformance found by @jerone with missing space
* Remove one of my stale line notes

Tested on dev okay

Closes OpenUserJS#428
Martii added a commit that referenced this pull request Nov 18, 2014
Abstract Production vs Development

Auto-merge
@Martii Martii merged commit 7b044da into OpenUserJS:master Nov 18, 2014
@Martii Martii deleted the Issue-428abstractProVsDev branch November 18, 2014 05:22
Martii pushed a commit to Martii/OpenUserJS.org that referenced this pull request Nov 18, 2014
@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

Did you just bloat every single file with 3 lines? All to import 3 globals (basically what they are) you're not using because they're not even needed in that file?

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

Just a note in case someone is new to debug mode... it can be invoked by: (correction here)

$ node debug app.js

then type c for continuous or n for the next line.... there are a few other commands available.


UPDATE 2021 04 17
node debug app.js is deprecated... use:

$ node inspect app.js

Same commands available for continuous etc.

See also:

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

Did you just bloat every single file with 3 lines? All to import 3 globals (basically what they are) you're not using because they're not even needed in that file?

Yes we did... start using them... sizzle is open to debug/dev messages, within limits of course, and more specific production console messages... eventually these could be tracked in a request log from morgan. It's also not our fault that V8 is still in the dark ages with ES5 strict and doesn't support ES6 destructuring arrays/objects yet... in which it would most likely be one line... anywho ... this is also useful for testing/debugging/development and production... obviously use at your discretion but the time of only a few coders understanding the flow of our project is over... this was discussed for many weeks and was one of the primary reasons why some ppl (including initially myself) are/were confused on our project.

If you are thinking we should have used the global Object... we don't want all modules to have access.

Anyhow... use it if you need it otherwise learn to "ignore" it much like the Hungarian App notation.

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

Btw eventually we'll have a little more modification to this but the base structure is this... and you'll probably hate it but I think you'll learn to love it. ;) :)

@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

Node 11 which isnt the stable release has your fancy es6 stuff. Dictionary unpacking wouldnt make this any better becuase itd still be bloating up files with 1 useless line.

My point is dont import it in files that do not use it.

Adding bloat doesnt solve the code organisation problems.

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

My point is dont import it in files that do not use it.

I appreciate your feedback on this but this is something that has been in the works since day one (and probably before)... and especially for the flow of the program. Having a select few understand this is unacceptable... a new issue or more is going to be created shortly to add to this but the base structure stays. If you can find a secure method of doing this cleaner that is fine but the structure must be compatible with ES6 destructuring and obviously not in everyone elses scope. This is part of the _template.js and all .js for now.

Both sizzle and I knew you would have an issue with this but I've been debugging code for over 3 decades now and this is integral to application development and is highly absent in nodes infancy coding.

@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

Ive already solved this twice in two different branches. I made settings.json into config.js mainly to be able to add comments, but also to include logic.

1 inport (on files that actually need it otherwise its dead code). Which you can access with config.isPro. Dictionary expansion is nice, but not required.

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

Well I took the perceived mutual idea to sizzle and he wants to keep settings.json specific to db... e.g. it should probably be renamed to dbSettings.json... but that's another issue... We also need to abstract out the port/host for IPv6 a little better with another json... I already did a preliminary branch privately just to play around with this... I wanted to ask you if you really need the environment variables... it's currently optional to remove these... but since I know you probably use bash to configure I wanted your input... but that's another upcoming issue.

Firstly we need to get less or whatever CSS compiler implemented because I want all external dependencies in /redist ... this includes bootswatch... I'm working with sizzle to assist him in changing deployment which like I said before probably needs to have our tree bumped down one level... now that I have some deploy experience with jitsu I can assist... but coordinating with sizzle is still an issue since he's quite busy.

@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

Making it a js file allows us to write comments for our silly hardcoded numbers and strings.

https://github.com/Zren/OpenUserJS.org/blob/pushdescription/config.js

Env variables are a requirement. You dont hard code sensitive info into the project. ever. It also allows you to test in a different env easier.

@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

An example config.js from a welll written project.

https://github.com/jedireza/drywall/blob/master/config.example.js

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

I wasn't planning on hardcoding anything other than localhost and 8080 in an additional .json... the precedence could still be environment variables (which are a security risk due to server side exploits) but I'm willing to yield if you are that adamant about using it for your bash configuration scripts... then the .json... then hard-coded (which it already is this part).

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

Making it a js file allows us to write comments for our silly hardcoded numbers and strings.

Regarding this... the "secrets" are stored as environment variables on nodejitsu (which resembles a .json btw)... so I'm not entirely sure that can work.

@Zren
Copy link
Contributor

Zren commented Nov 18, 2014

If you want default values if an env var is undefined, just use process.env.PORT || 8080. Again only possible with a js file. There is no reason at all for using a json file over a js file.

@Martii
Copy link
Member Author

Martii commented Nov 18, 2014

There is no reason at all for using json files.

Yes there is and environment variables are highly vulnerable to server side exploits (and they are already stored as .json like as I said above)... but like I said above that can be an upcoming issue... just wanted some preliminary input from you since I think you are the one that requested it months back for running the app on your intranet.

@Martii Martii added the CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. label Nov 18, 2014
@Martii Martii added this to the #249 milestone Nov 18, 2014
Martii added a commit that referenced this pull request Jan 12, 2015
Martii referenced this pull request in Martii/OpenUserJS.org Jan 12, 2015
Martii pushed a commit to Martii/OpenUserJS.org that referenced this pull request Feb 21, 2015
* Add visible distinction for debug and development modes... using short entries in case font is blocked so it doesn't bust layout... applies to OpenUserJS#429
* Assume pro as default... also pro can be kicked into debug mode which is one of the reasons this has been separated
* Some STYLEGUIDE.md conformance
* Remove line note... while `aFn` is still non-descript this is an exception for lack of better naming... try to avoid these though.
* This also is an additional perk for @TimidScript or others to reuse the `span` attribute for better site integration... consider this a sub-brand to maintain the integrity of the OUJS brand.
Martii added a commit to Martii/OpenUserJS.org that referenced this pull request Apr 17, 2021
* Also at some point *node* changed the way to detect if debugging was in process. Updated original comment at OpenUserJS#429 (comment) Been a while since I've been in this mode for deep debug examination.
* Fix missing custom headers for UA. This is optional but didn't know this until now that it had that option for the *github* dep. Post OpenUserJS#1753

Applies to OpenUserJS#1705

Ref(s):
* https://nodejs.org/api/inspector.html#inspector_inspector_url
* https://nodejs.org/api/debugger.html
Martii added a commit that referenced this pull request Apr 17, 2021
* Also at some point *node* changed the way to detect if debugging was in process. Updated original comment at #429 (comment) Been a while since I've been in this mode for deep debug examination.
* Fix missing custom headers for UA. This is optional but didn't know this until now that it had that option for the *github* dep. Post #1753

Applies to #1705

Ref(s):
* https://nodejs.org/api/inspector.html#inspector_inspector_url
* https://nodejs.org/api/debugger.html

Auto-merge
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment.
Development

Successfully merging this pull request may close these issues.

Abstract the production vs development code detection
2 participants