Skip to content
This repository has been archived by the owner on Nov 18, 2024. It is now read-only.

added __self to fbt ignored props #137

Closed
wants to merge 1 commit into from

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Apr 8, 2020

Summary

Recently, a bugfix went out for babel-plugin-transform-react-jsx-self that moves adding the __self JSX attribute before the arrow function transform. This has the side effect of adding the __self attribute before the fbt transform as well. This PR marks __self as a passed through but purposefully ignored attribute so that this change does not cause fbt to throw because of an unknown attribute

Test plan

I wrote a test to verify that the attribute is ignored. I also manually changed the code in the WWW repo and verified running the self transform in conjunction with this transform no longer throws an error.

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Apr 8, 2020
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kayhadrin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@kayhadrin kayhadrin self-requested a review April 8, 2020 23:45
@facebook-github-bot
Copy link
Contributor

@lunaruan merged this pull request in 8d997f6.

@kayhadrin
Copy link
Contributor

kayhadrin commented Apr 22, 2020

FYI: this PR has been reverted in c4468bd

@jorisre
Copy link

jorisre commented Apr 28, 2020

@kayhadrin Without this fix I got the following error :

Error trace
    ERROR in ./src/pages/index.tsx
    Module build failed (from ./node_modules/babel-loader/lib/index.js):
    Error: /Users/joris/workspace/my-project/src/pages/index.tsx: Invalid option "__self". Only allowed: project, author, preserveWhitespace, subject, common, doNotExtract 
    ---
    _this
    ---
        at errorAt (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/FbtUtil.js:232:10)
        at checkOption (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/FbtUtil.js:146:11)
        at /Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/FbtUtil.js:211:25
        at Array.forEach (<anonymous>)
        at getOptionsFromAttributes (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/FbtUtil.js:186:18)
        at JSXFbtProcessor._getOptions (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/babel-processors/JSXFbtProcessor.js:133:9)
        at JSXFbtProcessor.convertToFbtFunctionCallNode (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/babel-processors/JSXFbtProcessor.js:367:21)
        at PluginPass.JSXElement (/Users/joris/workspace/my-project/node_modules/babel-plugin-fbt/index.js:121:31)
        at newFn (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/visitors.js:179:21)
        at NodePath._call (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/path/context.js:55:20)
        at NodePath.call (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/path/context.js:42:17)
        at NodePath.visit (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/path/context.js:90:31)
        at TraversalContext.visitQueue (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/context.js:112:16)
        at TraversalContext.visitMultiple (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/context.js:79:17)
        at TraversalContext.visit (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/context.js:138:19)
        at Function.traverse.node (/Users/joris/workspace/my-project/node_modules/@babel/traverse/lib/index.js:84:17)
     @ multi ./src/pages/index.tsx static/development/pages/index.js[0]

I'm using Next.Js with Typescript, if needed I can provide an example repository.

@kayhadrin
Copy link
Contributor

When are you meeting this issue?
A test repo would be great.

I think this can be fixed by amending some configuration:
I.e. Configure the babel-plugin-fbt to accept __self as an extra option that'll be eventually ignored.

[ // when adding this babel plugin to the list of transforms
  require('babel-plugin-fbt'),
  {
    extraOptions: {__self: true},
  },
]

@jorisre
Copy link

jorisre commented Apr 28, 2020

Option extraOptions: {__self: true} worked ! Thanks
Maybe we can add this info in troubleshooting section of the doc ?

But now I'm getting the same error as #72 (comment) or #115 (comment) or #49 (comment)

Do you know how to fix it ?
Thanks

./src/pages/index.tsx
Error: /Users/joris/workspace/my-project/src/pages/index.tsx: Line 112 Column 9: fbt is not bound. Did you forget to require('fbt')?
---
fbt("Your FBT Demo", "title", {
  "__self": _this
})
---

@kayhadrin
Copy link
Contributor

Glad it worked for you.

Regarding that new error fbt is not bound. Did you forget to require('fbt')?, this is unrelated to this topic.

I'd suggest following up on #115 and adding a minimum repro repo for this.
It's especially important to know what JS code is being provided to the babel-plugin-fbt plugin (before processing) and how it differs from the original source code.

@yrichard
Copy link

yrichard commented Jun 5, 2020

I am still having this issue when using FbtParam: Invalid option "__self". Only allowed: number, gender, name.
Patching the code with this diff solves the problem, but this isn't the best solution long term.
You can use this repo for a repro case: https://github.com/yrichard/fbt-cra-typescript-installer-example

@AndrewCraswell
Copy link

AndrewCraswell commented Sep 6, 2020

@yrichard Did you ever find a more long-term solution to this? I've encountered the exact same issues with FbtParam and FbtPlural. I've also manually applied the patches in this PR and it resolves the issue with FbtParam but not FbtPlural. And I can confirm I've tried @kayhadrin's solution to add extraOptions, and while that does improve things in other areas (like using <fbt>), it doesn't solve the issue for other components.

@jrwats Any chance this might be reopened and merged again soon? It wasn't clear why it had been reverted

@mrtnzlml
Copy link
Contributor

mrtnzlml commented Feb 24, 2021

Hola, I'd like to bring this issue up again because the param usage still seems to be broken:

<fbt desc="summary of selected items in POS">
  Selected <fbt:param name="totalSelectedItems">{stats.totalSelectedItems}</fbt:param> items
  for <fbt:param name="totalPrice">{stats.totalPrice}</fbt:param>
</fbt>

Throws the following error:

Invalid option "__self". Only allowed: number, gender, name 
---
this
---

Here is my Babel config:

module.exports = {
  presets: ['…'],
  plugins: [
    [
      'babel-plugin-fbt',
      {
        extraOptions: {
          __self: true,
          __source: true,
        },
      },
    ],
    'babel-plugin-fbt-runtime',
  ],
};

The most common <fbt /> tags work fine. One workaround I found is not to use JSX for params, so:

{fbt(
  `Selected ${fbt.param(
    'totalSelectedItems',
    stats.totalSelectedItems,
  )} items for ${fbt.param('totalPrice', stats.totalPrice)}`,
  'summary of selected items in POS',
)}

But obviously, that's not ideal. Would you accept this PR again @jrwats? Or what was the issue with it?

@alexandernanberg
Copy link
Contributor

Created an issue for this #197

@jrwats jrwats self-assigned this Mar 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed Do not delete this pull request or issue due to inactivity. external_contributor Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants