Shrinkwrap in release

[sgress454]

From @Enteee on February 13, 2017 9:8

Overwriting dependencies of this adapter using npm-shrinkwrap seems to be impossible since you added a complete shrinkwrap file to the 0.12.x - release. Is there a reason for the shrinkwrap file beeing there? If not, i'd suggest removing it again.

Copied from original issue: balderdashy/sails-postgresql#269

[sgress454]

From @sailsbot on February 13, 2017 9:8

@Enteee Thanks for posting, we'll take a look as soon as possible. In the meantime, if you haven’t already, please carefully read the issue contribution guidelines and double-check for any missing information above. In particular, please ensure that this issue is about a stability or performance bug with a documented feature; and make sure you’ve included detailed instructions on how to reproduce the bug from a clean install. Finally, don’t forget to include the version of Node.js you tested with, as well as your version of Sails or Waterline, and of any relevant standalone adapters/generators/hooks.

Thank you!

[sgress454]

@Enteee can you clarify why it is you're trying to override the dependencies? Is there an important patch to one of the dependencies of sails-postgresql?

[sgress454]

From @Enteee on February 20, 2017 9:3

@sgress454 As packing a shrinkwrap is afaik bad practice i do think that the why should not be of importance. But i'd still like to give you some insights. It's mostly because of:

• Allow use of Group By without calculations waterline-sequel#100
• Injection safe waterline-sequel#101

But maybe this issue resolves itself if you have some profound resons for releasing versions with a shrinkwrap.

[sgress454]

From @particlebanana on March 1, 2017 21:50

Hey @Enteee we included the shrinkwrap file because we were having issues with dependencies of modules even when we had our own dependencies pinned. Using the shrinkwrap allowed us to guarantee that the module worked the way we expected it to. I'm not sure if thats bad practice or not but it was helpful for us.

If there are no longer issues and the test continue to pass, I can remove the shrinkwrap.json file if needed from the stable branch which is currently the 0.11 branch.

As part of the upgrade to Sails 1.0 and Waterline 0.13 we have abandoned the use of rolling our own sql generator in favor of using Knex which is much more widely supported and frees us from having to handle that part of the query process. We also have a new lower level query syntax called statements that support the kind of groupBy clauses you are attempting to perform as seen here.

By using that syntax and Knex to build parameterized statements they should be safe from injections.

[Enteee]

@particlebanana 👍 for migrating to knex. The current sql-builder raises quite a few concerns.

Anyway I opened this issue because commiting a shrinkwrap.json is bad practice in my opinion. I've found a "workaround" which solves my aggregation problem: Implementing the aggregations using Model.query().

when do you plan on releasing sails 1.0 & Waterline 0.13?

[sailsbot]

@sgress454,@Enteee: Hello, I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message and simply close the issue if possible. On the other hand, if you are still waiting on a patch, please post a comment to keep the thread alive (with any new information you can provide).

If no further activity occurs on this thread within the next 3 days, the issue will automatically be closed.

Thanks so much for your help!