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

NestJS GraphQL like for discovering Cypher files #34

Open
nartc opened this issue May 31, 2020 · 16 comments
Open

NestJS GraphQL like for discovering Cypher files #34

nartc opened this issue May 31, 2020 · 16 comments
Labels

Comments

@nartc
Copy link
Contributor

nartc commented May 31, 2020

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 jasperblues added bug Something isn't working high-priority labels Jun 1, 2020
@jasperblues
Copy link
Member

jasperblues commented Jun 8, 2020

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

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

@nartc
Copy link
Contributor Author

nartc commented Jun 8, 2020

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

@myflowpl
Copy link
Collaborator

myflowpl commented Jun 9, 2020

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
Copy link
Collaborator

myflowpl commented Jun 9, 2020

@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
Copy link
Member

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')

  • If the current persistence manager is for a Neo4j connection and routesBetween-NEO4J.cypher exists, 'inject' that.
  • Otherwise load routesBetween.cypher generic version.

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
Copy link
Member

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
Copy link

phyllisstein commented Nov 11, 2020

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.
/***/ "./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);
  
  /***/ }),
/***/ "./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
Copy link
Member

jasperblues commented Nov 12, 2020

@phyllisstein I would happily take a pull request.

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

@phyllisstein
Copy link

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
Copy link
Member

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
Copy link

phyllisstein commented Jan 28, 2021

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
Copy link

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

{
  ...
  "compilerOptions": {
    "assets": ["**/*.cypher"],
    "watchAssets": true
  }
}

@jasperblues
Copy link
Member

jasperblues commented Jul 2, 2021

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

@yinonburgansky
Copy link

@jasperblues yes, sounds good to me.

@maisonarmani
Copy link

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

@jasperblues
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants