liberation-data / drivine

Best and fastest graph database client (Neo4j, AgensGraph, Apache AGE, FalkorDB) for Node.js & TypeScript.
https://drivine.org
Apache License 2.0
153 stars 33 forks source link

NestJS GraphQL like for discovering Cypher files #34

Open nartc opened 4 years ago

nartc commented 4 years ago

https://docs.nestjs.com/graphql/quick-start#schema-first

Currently, new NestJS apps that are created by the CLI uses Webpack under the hood to build and run the application even in dev mode (off of dist folder). Hence, the current approach of using directory path for @InjectCypher() doesn't work unless users copy the .cypher files manually (or alter NestCLI webpack config).

Utilizing an approach like typePaths from nestjs/graphql might be a better solution for .cypher

jasperblues commented 4 years ago

https://drivine.org/guide/#/injection-decorators

^-- In the meantime I have put a workaround, described above. Does it help?

nartc commented 4 years ago

I will give it a test as soon as I have some free time either today or tomorrow. Thanks!

myflowpl commented 4 years ago

It looks like this one is related with #47 and has the same problem

47 was easy to fix, but injecting cypher files is different story and will require to have breaking changes

and as @nartc suggested, nestjs/graphql approach will be probably the best one to adopt here

myflowpl commented 4 years ago

@jasperblues Do we have to use DI for this ?

Lets change the CypherStatement interface:


//from
export interface CypherStatement extends Statement {
    text: string;
    language: 'CYPHER';
}

// to
export interface CypherStatement extends Statement {
    text?: string;
    file?: string | string[];
    language: 'CYPHER';
}

then usage will be:

const routesBetween = new CypherStatement({file: './routes-between'})
// OR if you need to user __dirname
const routesBetween = new CypherStatement({file: [__dirname, './routes-between']})

return new QuerySpecification<Route>()
       .withStatement(routesBetween)

and then let the CypherStatement load the file and may be cache it

jasperblues commented 4 years ago

That looks good. Another feature that I was considering is #39, which is to load a platform specific version, if it exists, for example @CypherStatement('./routesBetween')

I hadn't worked out exactly how to do it yet. But I was thinking statement would perhaps turn into a class and carry all the way through, until the time it is used by the connection, at which point it will ask for the best candidate.

jasperblues commented 4 years ago

Workaround

I will provide an official solution soon, in the meantime can you please write your queries inline (in the code)? For example:

const spec = new QuerySpecification(`MATCH (n) return (n)`)

Alternatively, you might modify your build/watch script to copy those files over. If using watch it could be a bit tricky, but might look something like:

Package.json:

"watch-dev": "rimraf dist && cross-env NODE_ENV=development ./node_modules/.bin/tsc-watch --onSuccess \"copyCpyherFiles.sh""

. . where copyCypherFiles.sh is an exercise for the reader at this point.

phyllisstein commented 4 years ago

You can hammer together a Webpack-idiomatic workaround with a small config tweak and a hacky custom decorator.

// webpack.config.js

yourConfig.module.rules.push[
  {
    test: /\.cypher$/,
    type: 'asset/source'    // Use raw-loader in Webpack <5.
  }
]
// require-cypher.decorator.ts

import { createParamDecorator } from '@nestjs/common'

export const RequireCypher = createParamDecorator(
  (pathname: string): string => {
    // The relative path and extension must be defined in the `require`
    // statement so that Webpack can create a context module.
    const relativePathname = pathname.replace(/^\.\//, '').replace(/\.cypher$/, '')

    const cypher = require(`./${ relativePathname }.cypher`) as string

    return cypher
  },
)
// actor.repository.ts

import { RequireCypher } from './require-cypher.decorator'

@Injectable()
export class ActorRepository {
  constructor(
    @RequireCypher('./movies-for-actor.cypher') readonly moviesForActor: CypherStatement,
  ) {}

ETA: This will get a little hairy if you try to work with Cypher files outside the current module scope. For instance, @RequireCypher('./movies-for-actor.cypher') works, but if you try @RequireCypher('../movies-for-actor.cypher') you're gonna have a bad time. I can dig into Webpack contexts more deeply this weekend.

Built asset example. ```js /***/ "./actor.repository.ts": /***/ ((__unused_webpack_module, __webpack_exports__, __webpack_require__) => { let ActorRepository = (_dec = (0,_nestjs_common__WEBPACK_IMPORTED_MODULE_4__.Injectable)(), _dec(_class = (_temp2 = (_temp = class ActorRepository { constructor(moviesForActor) { this.moviesForActor = moviesForActor; } async findByName(name) { const spec = new _liberation_data_drivine__WEBPACK_IMPORTED_MODULE_3__.QuerySpecification().withStatement(this.moviesForActor).bind({ name }); } }, _temp), _temp = (0,_require_cypher_decorator__WEBPACK_IMPORTED_MODULE_5__.RequireCypher)('./movies-for-actor.cypher')(_temp, undefined, 0) || _temp, _temp2)) || _class); /***/ }), ``` ```js /***/ "./movies-for-actor.cypher": /***/ ((module) => { "use strict"; module.exports = "MATCH (actor:Person {name: $name})-[:ACTED_IN]->(movies)\nWITH actor, collect(properties(movies)) AS moviesList\nRETURN {\n actor: {name: actor.name, born: actor.born},\n movies: moviesList\n }\n"; /***/ }), ```
jasperblues commented 4 years ago

@phyllisstein I would happily take a pull request.

And if you're willing, I feature contributors on the website and in the readme.

phyllisstein commented 4 years ago

Absolutely! Thanks for the nudge—it probably wouldn't have occurred to me 🙃 . The glitch with relative paths is worth at least a little more attention. But I'll definitely kick a PR your way if I can fix it.

jasperblues commented 4 years ago

Even if we can reduce the number of folks needing to use a work-around that will add value, and it can be improved incrementally.

phyllisstein commented 3 years ago

I (finally) spent some time digging through Drivine's strategy for injecting Cypher files. It's quite clever, and I'm sure it could be extended for Webpack. I'm curious, though, whether this might be a more reasonable solution, from your perspective and from other users'.

Before:

@Injectable()
export class TreeRepository {
    constructor(
        @InjectPersistenceManager()
        readonly persistenceManager: PersistenceManager,
        @InjectCypher(__dirname, 'cypher/create-tree')
        readonly createTree: CypherStatement,
        @InjectCypher(__dirname, 'cypher/get-all-trees')
        readonly allTrees: CypherStatement,
    ) {}

    // --- ✁ snip

After:

@Injectable()
export class TreeRepository {
    private createTree = require<CypherStatement>('./cypher/create-tree.cypher')

    private getAllTrees = require<CypherStatement>('./cypher/get-all-trees.cypher')

    constructor(
        @InjectPersistenceManager()
        readonly persistenceManager: PersistenceManager,
    ) {}

Webpack >=5 users would need to add the following to their config:

module.exports = {
    // --- ✁ snip
    module: {
        rules: [
            // --- ✁ snip
            {
                test: /\.cypher$/,
                type: 'asset/source',
            },
        ],
    },
}

Webpack <5 users would do something similar with raw-loader. (Q.v. the docs.)

Those bits of config tell Webpack that it should recognize .cypher files in require calls and import them as strings—which Drivine is more than happy to consume.

I'm not well-enough versed in Nest to know whether skirting DI like this is idiomatic or acceptable. But it's the least convoluted solution I've tried by a mile.

yinonburgansky commented 3 years ago

According to NestJS Docs: we can copy assets to the dist directory by modifying nest-cli.json:

{
  ...
  "compilerOptions": {
    "assets": ["**/*.cypher"],
    "watchAssets": true
  }
}
jasperblues commented 3 years ago

@yinonburgansky so we simply need to update the Drivine docs to make it clear? Does it solve the issue?

yinonburgansky commented 3 years ago

@jasperblues yes, sounds good to me.

maisonarmani commented 2 years ago

@jasperblues I don't think the doc has been updated for some wierd reason.

jasperblues commented 2 years ago

Sorry folks, I haven't had much time for opensource work lately. If you need the docs updated can someone send a PR?

Perhaps we can move the docs back to the wiki as well, if that would make them easier to maintain.