nfroidure / svg-pathdata

Parse SVG PathDatas
MIT License
183 stars 18 forks source link

Write parser and encoder in pure functions #34

Closed pioug closed 6 years ago

pioug commented 6 years ago

Fixes #31 and #33

Opening the PR for visibility. But it is still ongoing

Proposed changes

Code quality

License

To get your contribution merged, you must check the following.

Join

My NPM username: pioug

NaridaL commented 6 years ago

Hey @pioug thanks for the work, looks mostly good so far. Unfortunately, the "streaming" part, as exemplified by the following example from the readme:

parser.write('   ');
parser.write('M 10');
parser.write(' 10');
// {type: SVGPathData.MOVE_TO, relative: false, x: 10, y: 10 }

(i.e. you can pass strings in chunks) is no longer possible.

Changing it to fix that shouldn't be too difficult, basically you want to to model the parser state type ParserState = {curNumber: string, curCommand: Command, state: number} as an explicit object, which you optionally accept as a parameter in the pure function and return in addition to the parsed commands so the user can continue parsing with the current parser state.

To cite @nfroidure's original issue:

function parseSVGPathData(stateObject: ParserState = START_PARSER_STATE, chunk: string) {
  // Parsing

  return [
    newStateObject,
    commands
  ];
}

Personally, I'd modify it to parseSVGPathData(chunk: string, stateObject: ParserState = ..., commands: Command[] = []) so that you can pass the previous commands array to have the function append the new commands to. While that isn't perfectly pure, it seems practical. I'll leave it up to you ;-)

NaridaL commented 6 years ago

Alternatively, instead of going the pure function way, you could just implement it the way the transformers currently are (with closures):

function createParser() {
  let curState =
  let curNumber =
  let curCommand = 
  return function parseChunk(chunk: string, commands: Command[] = []) {
    commands.push(...)
    return commands
  }
}

This would have the advantage of consistency. Rewriting all the transformers as pure functions seems like a lot of work. (Although if you decide to just have the parser as a pure function and leave the transformers as they are I'll still merge it...)

EDIT:

the readme example would then look as follows:

const parser = createParser()
parser('    ') // returns []
parser('M 10') // returns []
parser(' 10') // returns [{type: SVGPathData.MOVE_TO, relative: false, x: 10, y: 10 }]
NaridaL commented 6 years ago

Also, the "stream" import in SVGPathDataTransformer.ts should also be removed.

pioug commented 6 years ago

@NaridaL Thanks a lot for reviewing 🙏 I will find some time to update but feel free to commit any meanwhile!

NaridaL commented 6 years ago

@pioug I finished the PR and committed it. Thanks a lot for your help!