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 get/set/delete a script source with a script object... and use it. #390

Open
Martii opened this issue Oct 23, 2014 · 3 comments
Open
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. later A long time ahead, in a galaxy near, near... mightfix Ehh maybe... convince us all...
Milestone

Comments

@Martii
Copy link
Member

Martii commented Oct 23, 2014

I'm going to "override" @sizzlemctwizzle on this but still keep it in line with the current HEAD deployment to keep this from being hung like it was in #150

e.g. this is part of the needed optimization and can be used with migration routines.

Zren has a commit at Zren@c31a0b0#-P0 that does part of this however his branch may be too murky to merge.


Some misc notes:

  • Replace existing routines with actual usage of these functions
  • At this time keep the abstraction layer between mongoose and our code e.g. avoid at this time using methods in the models themselves in response to Hotfix for a potential security issue #686.
@Martii Martii added enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. DB Pertains inclusively to the Database operations. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Oct 23, 2014
Martii referenced this issue in Martii/OpenUserJS.org Oct 23, 2014
* Works on dev
* There is probably some redundant, unused, mustache options set since this is merged/modified code from `script.js` e.g. not efficient at the moment.
* Breadcrumbs are slightly off position wise due to the sidebar being absent

Perhaps the fix needed for OpenUserJS#150. This is my least familiar portion of the code but trying to learn it now... hopefully with the help of some others. :)
Martii referenced this issue in Zren/OpenUserJS.org Aug 5, 2015
Created scriptStorage.script_getSource(script, callback) that accept a script object rather than do a second query on the db when we already have it.
@Martii Martii changed the title Add get/set/delete a script source with a script object. Add get/set/delete a script source with a script object... and use it. Aug 5, 2015
@Martii Martii added the sooner Sooner would be appreciated. label Aug 7, 2015
Martii pushed a commit to Martii/OpenUserJS.org that referenced this issue Aug 18, 2015
* Misplaced `var` declarations ... closer to current ES5.x implementation for some others just misplaced
* Reordered on first use for all vars... not as critical but good for debugging
* Changed `s3` early initialization... shouldn't need to be **and if it did** it should always be `NOTE:`d with a comment
* Some newlines for white space usage... easier reading/debugging
* Any `require` statements all at top... `require` functions become cached and always stay loaded after first use... so really shouldn't be attempting to optimize these in our code... only perceived benefit is startup pm time but is loaded elsewhere so no real benefit.
* Some notation of `WARNING:` statement on incomplete error handling
* White-space trim that my editor didn't catch on a previous commit even though it is on all the time everywhere. :\
* `WARNING:` note added for resaving script model object for no apparent reason... again should be notated.
* Max line length of 100 conformance

Applies with OpenUserJS#285, OpenUserJS#486, OpenUserJS#390, and OpenUserJS#19 ... perhaps some more... very minimal change in project behavior here which is why it is isolated.
@Martii
Copy link
Member Author

Martii commented Aug 18, 2015

So in your commit @Zren were you planning on not using a stream buffer when searching (for) the metadata block(s)? Perhaps I'm missing the point today so please humor me as I'm still warming up from a much needed break in real life.

In short we should be ready to do this next but if you would like to show me how you wanted to do this to reassert what you want in a PR I'd be just about ready for that. Please limit it to this issue... and it may take me a bit more to fully comprehend everything you were looking for. One minor exception is as far as placing the getters and setters in the models please leave them abstracted from mongoose, etc... e.g. don't put them in the models as I would prefer not seeing a Script object created, even if temporary, every time we need to access these methods (getters and setters) ... I've barely looked into hooks with mongoose as well but that could be an option at a middle compromise point.


Found target code points are currently .../scriptStorage.js and .../user.js

@Zren
Copy link
Contributor

Zren commented Aug 18, 2015

were you planning on not using a stream buffer when searching the metadata block(s)?

Probably? It was just easier to deal with as a single string than an array of buffers. I never did any performance testing with either case though. The github library (for github import) returned a string, so I needed a way to parse the metadata with a string and it seemed silly to convert it to a Buffer[] just so I could use the getMeta function.

TBH, the GitHub import feature isn't really needed since the user should just copy paste the code when setting up a new script.

@Martii
Copy link
Member Author

Martii commented Aug 18, 2015

I never did any performance testing with either case though.

It is useful when the metadata block is at the top of a file otherwise it is a relooped routine which seems pretty fast... I had a brief concern with @sizzlemctwizzle in private about a possible pitfall in #285 with the blocks, if not used... it's an extra perk in processing a shorter amount of data with the buffer when all (blocks) are present (at the top)... but I realize both this issue and the other one may negate that perk. The benefits of not colliding on key names is one of the ultimate goals of the other issue besides localization support.

TBH, the GitHub import feature isn't really needed since the user should just copy paste the code when setting up a new script.

I routinely delete Unit Tests on OUJS and reimport from GH so it would nice to keep that in instead of a paste operation... portable devices aren't as easy to do this as well... also if we run into a sync failure issue down the line we can resync scripts manually until it is a little more solid code between Organizations and single use accounts here on GH. (I had to do this once with a connection failure of S3 where script source wasn't showing up but Script objects were updated)

@Martii Martii removed the sooner Sooner would be appreciated. label Aug 30, 2015
@Martii Martii self-assigned this Mar 23, 2016
@Martii Martii added mightfix Ehh maybe... convince us all... later A long time ahead, in a galaxy near, near... labels Mar 23, 2016
@Martii Martii added this to the #644 milestone Mar 24, 2016
@Martii Martii removed their assignment May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. DB Pertains inclusively to the Database operations. enhancement Something we do have implemented already but needs improvement upon to the best of knowledge. later A long time ahead, in a galaxy near, near... mightfix Ehh maybe... convince us all...
Development

No branches or pull requests

2 participants