-
Notifications
You must be signed in to change notification settings - Fork 18
Fix eslint warnings #141
Fix eslint warnings #141
Conversation
| describe('Nodes.js require', () => { | ||
| it('should work without needing to use `.default`', () => { | ||
| assert(require('../lib/fury').parse); | ||
| assert(fury.parse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change sounds like it makes the test not do what it is described to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's testing whether .parse works without .default. The fury is const fury = require('../lib/fury');, so it's equivalent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@honzajavorek Ah, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, although IMO I dislike the global version of Fury that is returned here. Personally I would like to consider dropping this functionality so that require('fury') returns the Fury class which the consumer must instansiate their own version and add the adapters. I don't like singleton state that is currently permitted.
Along with #32 we would be renaming the libraries and Fury would become API Elements Core. In that case, I think it would make sense to make "Fury" itself a Namespace and thus perhaps in the future we would have something like:
const { Namespace } = require('@apielements/core');
const APIBlueprintParser = require('@apielements/apib-parser');
const namespace = new Namespace();
namespace.use(APIBlueprintParser);
const parseResult = await namespace.parse({ source });As the biggest consumers of Fury (Dredd/Docs) I would love to hear some feedback regarding these potential changes. I was thinking that Namespace here is actually a subclass of minim namespace (if Minim continues to exist standalone). It is a namespace with additional methods detect, validate, parse and compose/serialise.
I have also been pondering the idea of a complete API Elements JS package which incorporates all the adapters for all formats so you don't have to worry about individual support.
const { Namespace } = require('apielements');
const namespace = new Namespace();
const parseResult = await namespace.parse({ source });In your cases it may make sense to depend on apielements whereas the separate core, and adapter packages are provided for consumers who wish to mix and match adapters. Although you both are not using the serialisers and this may add additional dependency bloat so it may not be desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool then! :)
There are some eslint exceptions that can be fixed easily.