Improve options validation

[alexprey]

At this point not all option parameters validated by the code. It will be nice to add checks for all options.

• Write tests and check all possible ways
• Add validation for all options
• Ensure that all tests passed

[soft-decay]

I started implementing the options validation in a new branch.
So far I added encoding and ignoredVisibilities and created typings for intellisense.

• How would like the tests organized? I was thinking to put them in unit/options/validation.spec.js.
• Should I move the validation code to a new file? I was thinking of moving options related code to lib/options.js (mainly validateOptions, normalizeOptions, and associated constants).
• Are you open to have feature branches in the future? Maybe for this feature, but I have Add support of TypeScript inside Svelte components #34 in mind when asking this.

Btw, thank you @alexprey for being really responsive and friendly, it's has been great collaborating with you, and I hope it continues.

[alexprey]

@soft-decay thanks for taking this task!

1. Yes, you can put tests by this path.
2. That is a great idea to move this logic at separate files. Now code structure in the parser are ugly and mostly placed in one file. I have plans to split logic by atomic modules, but not start yet.
3. Currently I don't have any feature branches open, but it's ok when start a new huge changes of the lib, like a TS support. When I'm start implementing a Svelte V3 support I use a branch for that.

Thanks you too for collaboration, I very appreciate your interest in this project!

[soft-decay]

Quick question.

In lib/helpers.js@loadFileStructureFromOptions there is this check:

if (options.structure) {
    return options.structure;
}

for the presence of a structure key in options which would look something like this :

content?: string;
template?: string;
scripts?: unknown[];
styles?: unknown[];

but I did not find where structure was assigned to options. In index.js@parse, the structure is loaded:

const structure = loadFileStructureFromOptions(options)

but then structure and options are passed around together. Is it ok if I do:

options.structure = loadFileStructureFromOptions(options)

then remove structure parameter in function signatures and use options.structure instead. I think it would be a little cleaner.

[alexprey]

This can be used when code of component was already splitted, f.ex. when we use a svelte preprocessor or webpack plugin we have already splitted content.
We use logic at the start of development in our build process for svelte v2 components. And this is a legacy API from original library vuedoc-parser, that was a base and example for first version of sveltedoc-parser.
Actually I'm not sure that it was still used, and it is not documented now, so maybe it can be marked as depricated.

[soft-decay]

Ok, I decided to assign directly to options.structure and remove the structure param passed around. In the end is it assigned the same in the parser constructor :

// file: lib/v3/parser.js
-   constructor(structure, options) {
+   constructor(options) {
        super();

        this._options = Object.assign({}, DEFAULT_OPTIONS, options);

-       this.structure = structure;
+       this.structure = options.structure;

[soft-decay]

I created pull request #44 for this.

I might have gone a little overboard with the options typings, but now there is pretty good autocompletion and type checking for the options object. I also documented the main parse function with a usage example.

I added basic tests for options, but maybe something more exhaustive is required.

[soft-decay]

I think the next step is to start validating parser specific options (inside each parser). I checked what is already validated, and each parser uses a different approach:

• v2-parser
  1. options is assigned default values;
  2. uses a static method to validate the options object;
  3. The parser's properties are then set using the values from options;
  4. options object is discarded.
• v3-parser
  1. options is assigned default values (empty object right now) and kept in this._options variable
  2. this._options is never used again;
  3. The parser's properties are then set using the values from options or an inline default value;

v2-parser: Constructor

A lot of things seem to be outdated here.

What to do with these :

this.defaultMethodVisibility = options.defaultMethodVisibility;
this.defaultActionVisibility = options.defaultActionVisibility;

They don't exist in options. Should we add these options, or maybe set a default value for them instead? I saw that 'public' is used as a default value later on, but it's a bit confusing.

It seems the DEFAULT_OPTIONS are for the espree parser, but they are assigned to the same options object. Maybe build a separate espreeOptions to avoid confusion and potential name collisions. Also some of these options seem to not even exist in the espree/acorn parser (e.g. attachComment)

v3-parser: Constructor

This is not used :

this.identifiers = {};

Todo

v2-parser:

• Cleanup espree options
• Better Validations
• Add Tests

v3-parser:

• Better Default Values
• Add Validations
• Add Tests

[soft-decay]

In the v2-parser constructor:

try {
  // try with default options { loc: undefined, range: false, comment: undefined }
} catch (e) {
  const script = utils.escapeImportKeyword(options.source.script);
  // try again with { loc: true, range: true, comment: true }
}

Is there a reason for this. Wouldn't it be better to set the default values to true and pre-escape the imports?

There is also this block :

if (this.ast === null) {
    if (this.features.includes('name')) {
        this.parseComponentName();
    }

    return this.emit('end');
}

The check for feature 'name' should be out of the block I think. It also appears at the end of this.ast.body.forEach(...), which is redundant if you put it at the start of the internalWalk method.

[alexprey]

Not sure that the whole block should be wrapped by feature check, because the end event should be always emitted when parsing are done

[soft-decay]

I just meant to do this:

if (this.features.includes('name')) {
  this.parseComponentName();
}

if (this.ast === null) {
  return this.emit('end');
}

and remove the same check present at the end of this.ast.body.forEach(...).

In all cases, I think I'll start putting my efforts on parser-v3 instead. It's the current version of svelte so it's probably the most used.