WIP: No children prop

[macovedj]

no-children-prop eslint rule

[ooflorent]

Could you hoist the closure to the top level?

[sebmck]

Also don't use as. We're losing type precision here for no reason and I suspect this will fail accessing an unknown property on JSXSpreadAttribute.

[ooflorent]

Could you also hoist this one?

[macovedj]

I assumed that you meant by these that you'd like me to define the functions at the top level, which I've done. Let me know if you meant something else?

[ooflorent]

I believe this fixable

[macovedj]

Are you thinking we would just get rid of the children prop? We could do that. I worry a little bit that people would maybe prefer to just be told though, so that they don't have to add it again with a name other than "children". But if you still think it would be better to delete it, or had something else in mind, then I could do that.

[macovedj]

When running tests, I'm being prompted to run scripts/dev-rome test packages/@romejs/js-parser/index.test.ts --update-snapshots which then updates two lines (column and index fields of an object) across 969 markdown files, and I already tried catching up with master to be rid of it. Should I just bring these updates in, or am I doing something wrong?

[Kelbie]

@macovedj I also got those when I created my PR but I just ommited them

[macovedj]

@macovedj I also got those when I created my PR but I just ommited them

Neato. New problem though. When I try to update snapshots for the tests that I wrote, I get this error which I'm having difficulty parsing.

`✖ unimplemented

10 │ export default function JSXReferenceIdentifier(): Token {

11 │ throw new Error('unimplemented');
│ ^
12 │ }

14. (packages/@romejs/js-compiler/lint/rules/react/noChildrenProp.test.ts:14:10)
15. ___R$project$rome$$romejs$js$compiler$lint$rules$testHelpers_ts$testLintMultiple (
    packages/@romejs/js-compiler/lint/rules/testHelpers.ts:29:10)
16. ___R$project$rome$$romejs$js$compiler$lint$rules$testHelpers_ts$testLint (
    packages/@romejs/js-compiler/lint/rules/testHelpers.ts:71:20)
17. ___R$project$rome$$romejs$js$compiler$lint$index_ts$default (
    packages/@romejs/js-compiler/lint/index.ts:50:24)
18. ___R$project$rome$$romejs$js$formatter$index_ts$formatJS (
    packages/@romejs/js-formatter/index.ts:41:24)
19. ___R$project$rome$$romejs$js$formatter$Builder_ts$default.tokenize (
    packages/@romejs/js-formatter/Builder.ts:68:22)
20. ___R$project$rome$$romejs$js$formatter$builders$core$Program_ts$default (
    packages/@romejs/js-formatter/builders/core/Program.ts:23:12)
21. ___R$project$rome$$romejs$js$formatter$Builder_ts$default.tokenizeStatementList (
    packages/@romejs/js-formatter/Builder.ts:136:25)
22. ___R$project$rome$$romejs$js$formatter$Builder_ts$default.tokenize (
    packages/@romejs/js-formatter/Builder.ts:68:22)
23. ___R$project$rome$$romejs$js$formatter$builders$statements$ExpressionStatement_ts$default
    (packages/@romejs/js-formatter/builders/statements/ExpressionStatement.ts:16:25)
24. ___R$project$rome$$romejs$js$formatter$Builder_ts$default.tokenize (
    packages/@romejs/js-formatter/Builder.ts:68:22)
25. ___R$project$rome$$romejs$js$formatter$builders$jsx$JSXElement_ts$default (
    packages/@romejs/js-formatter/builders/jsx/JSXElement.ts:23:12)
26. ___R$project$rome$$romejs$js$formatter$Builder_ts$default.tokenize (
    packages/@romejs/js-formatter/Builder.ts:68:22)
27. ___R$project$rome$$romejs$js$formatter$builders$jsx$JSXReferenceIdentifier_ts$default (
    packages/@romejs/js-formatter/builders/jsx/JSXReferenceIdentifier.ts:11:8)

ℹ Lint options

Object {
  category: 'lint/noChildrenProp'
  sourceType: 'module'
  syntax: Array []
}

ℹ Input

<MyComponent children={'foo'}></MyComponent>

`

[sebmck]

It means JSXReferenceIdentifier is missing a function to handle formatting, which is true. All that needs to be done is update packages/@romejs/js-formatter/builders/jsx/JSXReferenceIdentifier.ts#L11 to:

export default function JSXReferenceIdentifier(
  builder: Builder,
  node: JSXReferenceIdentifier,
): Token {
  return node.name;
}

Feel free to do that in this PR.

[sebmck]

When running tests, I'm being prompted to run scripts/dev-rome test packages/@romejs/js-parser/index.test.ts --update-snapshots which then updates two lines (column and index fields of an object) across 969 markdown files, and I already tried catching up with master to be rid of it. Should I just bring these updates in, or am I doing something wrong?

I believe there's a bug in snapshots that's not causing failures to be emitted correctly. There was a PR that must have been merged that incorrectly allowed them. I pushed the updated snapshots to master earlier.

[sebmck]

Looks good, thank you!