Present object properties as StringLiteral node when Identifier can not be used

[unconfident]

Fixes #18

[unconfident]

Your yarn.lock contains links from some private registry that responds with 403 code to outsiders. Because of this I decided to not add any additional dependencies to the project. I think if I were to do it I would have to override the entire lock file.

But you may want to do it. src/validateIdentifier.js could be replaced with is-valid-identifier package which uses same regular expression internally, just published to NPM as a proper package.

[wmonk]

Thanks for this! Which is the private dependency in the yarn lock file?

[wmonk]

@unconfident have checked the lock file and can see taobao as one of the registries - will open a PR to rebuild this so you can add is-valid-identifier.

[wmonk]

I have rebuilt the lock file in #20 - so you can rebase that once merged. Can you also add a test for the property generation.

[unconfident]

Hi and sorry for the late response.

Which is the private dependency in the yarn lock file?

It wasn't private dependency, but a private registry instead. At least from how it looked. When Yarn attempted to download modules from npm.goodeggs.com as was listed in lock file it's when I was getting 403 responses. But now its solved, I'll rebase my branch to latest state of the master branch, thanks.

Can you also add a test for the property generation.

I was thinking about adding a test for this, but seeing how FlowTypeGenerator.spec.js is missing from the tests directory I didn't know how to approach it. Covering whole file with tests seemed too ambiguous of a task. Comparing generated output character by character may be affected by literally every change made to the code and likely by future babel-generator releases. Generating code with compact, concise or minified options will likely help, but not much and will make it less readable.

Comparing AST trees will practically duplicate code of FlowTypeGenerator.js with slight changes and it's not a good way of testing stuff.

One way to do it is to attempt parsing generated code back with babylon, which will throw in case of a syntax error and produce an AST tree that can be inspected further if needed. I think this is what I'll go with

[unconfident]

Just learned that proposed change doesn't work with Babel 7. @babel/types insists that object type property can only be Identifier, which is false

TypeError: Property key of ObjectTypeProperty expected node to be of a type ["Identifier"] but instead got "StringLiteral"

Perhaps it's still too early to use it?

[wmonk]

I was thinking about adding a test for this, but seeing how FlowTypeGenerator.spec.js is missing from the tests directory I didn't know how to approach it.

Didn't realise this file had no tests on it - so fine to leave for now.

Perhaps it's still too early to use it? One of the reasons for the update to Babel 7 was to allow exact object types. In my opinion the benefit of having these outweigh the use of string identifier keys on object types. So would rather keep the upgrade, and help you fix the underlying issue in Babel.

Do you have an issue we can track? I have found the Babel team to be pretty helpful when submitting PRs, and getting them released.

[unconfident]

I tried to look for an issue describing this problem. Didn't find, so I created a new one: https://github.com/babel/babel/issues/7635

Regarding this pull request, I think I found a reason to not replace validateIdentifier.js with is-valid-identifier package. That regular expression checks for string not being a reserved word and while those can't be used for variable names they will work for an object key. If we skip that ckeck there will be less differences from the code generated by previous versions of this tool, so it may be preferable.

I also rebased branch and added test case. I went the route of parsing generated code with Babylon as I suggested previously. It validates that generated AST will produce no syntax errors when turned into code and that's exactly what we want to know. I also threw babel-code-frame to the mix, so when it does an error message will make it clear what exactly has gone wrong. Here's a fragment of ava -v output from revision f50dc43:

   75:                                      
   76:   t.notThrows(() => parseCode(code));
   77: });                                  

  Threw:

    [SyntaxError: Unexpected token, expected ":" (3:4)
      1 | type Type = {
      2 |   valid?: string,
    > 3 |   in-valid?: string,
        |    ^
      4 | };]

[unconfident]

@wmonk is everything good now?

[wmonk]

@unconfident thanks for your patience - I will get to this today! 👍

[wmonk]

@unconfident I will merge once the merge conflict is fixed

[unconfident]

Rebased to latest master. Ammended last commit to also include changes in /flow-typed. Not sure why, but signature has changed for some of the files without any other changes to them:

• flow-typed/npm/flow-bin_v0.x.x.js
• flow-typed/npm/lodash_v4.x.x.js

[wmonk]

Thanks for your work @unconfident 🎉