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

feat(utils): add makePgSmartTagsPlugin #541

Merged
merged 11 commits into from
Nov 7, 2019
Merged

Conversation

benjie
Copy link
Member

@benjie benjie commented Oct 14, 2019

This'll let you define our internal "tags" in JS or JSON/JSON5 rather than using "smart comments" in the database. After some discussion in the Graphile Team we've decided to call the internal tags "smart tags" to make them more searchable, less ambiguous, and to have continuity from the existing "smart comments".

If you want to store a JSON file with all your smart tags, you could do something like this in postgraphile.tags.json5:

{
  "version": 1,
  "config": {
    "class": {
      "my_table": {
        "tags": {"omit": "create,update,delete"},
        "description": "You can overwrite the description too",
        "columns": {
          "my_private_field": {"tags": {"omit": true}}
        }
      },
      "my_schema.myOtherTable": {
        "description": "If necessary, add your schema to the table name to prevent 
multiple being matched"
      }
    },
    "procedure": {
      "my_schema.my_function_name": {
        "tags": {"sortable": true, "filterable": true}
      }
    }
  }
}

Or you might want to add a smart tags plugin in JS to mark every table that matches a certain regexp as omitted:

module.exports = makePgSmartTagsPlugin({
  kind: 'class', // 'class' covers tables, views, materialized views and composite types
  match: name => /regexp_here/.test(name),
  tags: {
    omit: true
  },
});

Of course this doesn't only work for the omit smart tag; all smart tags work here. Smart tag values should be true, a string, or an array of strings (as specified in the smart comments spec). (Docs will be updated to reflect the difference between smart tags and smart comments at a later date.)

Identifiers are case sensitive. If multiple tables match the same identifier (e.g. same name but different schema) then they will all be affected by the rule unless the rule includes the schema name.

@benjie
Copy link
Member Author

benjie commented Oct 14, 2019

Note to self: to enable this to work in watch mode, we should add another helper that takes a filesystem path and watches it. Then live comments can be updated whenever the JSON is written.

@benjie
Copy link
Member Author

benjie commented Oct 14, 2019

Maybe add a version number to the file so we can change the format later.

@garcianavalon
Copy link
Contributor

This looks cool! Having this kind of configuration outside of db migrations and into version control sounds more robust. Right now a developer can easily override previous comments ands its hard to catch

@benjie
Copy link
Member Author

benjie commented Oct 15, 2019

Agreed. It’s already possible with fairly simple plugins but I figure making an explicit configuration format can’t hurt ;)

Do you think this is okay as a plugin, or should it maybe be part of core? I figured as a plugin you could split up your rules however you want over multiple plugins.

@garcianavalon
Copy link
Contributor

garcianavalon commented Oct 16, 2019

If the functionality it's the same, I don't feel very strong about either option.

From a dev perspective, it feels weird when every thing you need to do in a framework is a plugin you need to activate, but at the same time it's an optional configuration method. Adding it to core and keeping the current smart comment spec can feel weird too, as its seems that there is two methods to achieve the same thing without guidance on which one is recommended.

If it stays a plugin I would understand that the "standard" method is real comments in the database. If it gets added to core, I would understand that the "standard" method is JSON conf (it just feels more powerful) and that db comments are a "fallback" or legacy implementation.

If there is functionality difference, I would go for the one that leaves more doors open (I'm assuming both implementations have the same cost and tech drawbacks). In my experience building stuff for other to use, simpler is the better option, then the more flexible/less locking one

@benjie
Copy link
Member Author

benjie commented Oct 16, 2019

I think I'll keep it a plugin; we can factor it into a preset in v5.

@garcianavalon
Copy link
Contributor

Btw, I just noticed the description field. Will this plugin allow for adding extra docs to the GraphQL API? That would be awesome

@benjie
Copy link
Member Author

benjie commented Oct 16, 2019

Yeah; it basically can replace database comments.

@crubier
Copy link

crubier commented Oct 19, 2019

With the JS version, how would you add several kind of comments like exposed in the JSON version ?

@benjie
Copy link
Member Author

benjie commented Oct 19, 2019

I don’t follow; what do you mean exactly?

@benjie
Copy link
Member Author

benjie commented Nov 7, 2019

Overhaul:

  • Renamed to "smart tags" plugin rather than "smart comments" plugin. This way it's easy to distinguish between the two ways (we don't want an Apple TV-style issue).
  • Actually tested it 😉
  • Made it more flexibile and forgiving
  • Added watch mode without adding filesystem dependencies into graphile-engine (instead you'll have to do your own file watching - sorry - didn't want it conflicting with Lambda-mode)
  • Wrote integration in PostGraphile for it (will be in a separate PR) which auto-watches postgraphile.tags.json5
  • Changed the file format so that it has an extra level defining the version (1).

Will update the comment at the top shortly.

@benjie benjie changed the title feat(utils): makeSmartCommentsPlugin/FromJSON feat(utils): add makePgSmartTagsPlugin Nov 7, 2019
@benjie benjie marked this pull request as ready for review November 7, 2019 14:37
@benjie benjie merged commit 40a7bfa into master Nov 7, 2019
@benjie benjie deleted the smart-comments-plugin branch November 7, 2019 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants