detect on common react patterns #130
Conversation
|
Nice idea! I agree though, I know I personally would appreciate this change even more if it could be disabled via an option. While 99% of users importing from |
|
Right; this should be an option, and should also I think skip comments in addition to whitespace. |
|
Could you add an option to enable/disable this check? |
ftdetect/javascript.vim
Outdated
| autocmd BufNewFile,BufRead *.jsx set filetype=javascript.jsx | ||
| autocmd BufNewFile,BufRead *.js | ||
| \ if <SID>EnableJSX() | set filetype=javascript.jsx | endif | ||
| \ if <SID>EnableJSX() | set filetype=javascript.jsx | endif |
ftdetect/javascript.vim
Outdated
| if g:jsx_pragma_required && !b:jsx_pragma_found | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') && | ||
| \ !(get(g:,'jsx_prevalent_declaration') && search(s:jsx_prevalent_pattern, 'nw')) |
There was a problem hiding this comment.
Can you make this into a separate conditional? We could smush all three checks into one, but they're unrelated and I think it's clearer to separate them (and also probably avoids a line continuation).
There was a problem hiding this comment.
that condition is a pre-req. If it was rearranged, it'd be dead code because of the return
ftdetect/javascript.vim
Outdated
| if g:jsx_pragma_required && !b:jsx_pragma_found | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') && | ||
| \ !(get(g:,'jsx_prevalent_declaration') && search(s:jsx_prevalent_pattern, 'nw')) |
There was a problem hiding this comment.
I think it's a little strange to identify the declaration as "the prevalent declaration" rather than "the react import" or "the require react expression". Rename this variable to something like g:jsx_check_react_import or something more self-documenting.
ftdetect/javascript.vim
Outdated
| " importation, we guess the user writes jsx | ||
| " endif | ||
| let s:jsx_prevalent_pattern = | ||
| \ '\v\C%^\_s*%(%(//.*\_$|/\*\_.{-1,}\*/)@>\_s*)*%(import\s+\k+\s+from\s+|%(\l+\s+)=\k+\s*\=\s*require\s*\(\s*)[`"'']react>' |
There was a problem hiding this comment.
Just to confirm—this regex skips over comments and looks for something that looks like import ... from ... 'react or require( ... 'react (modulo whitespace, choice of quote, etc.). Is that correct, or have I missed something?
There was a problem hiding this comment.
that's it. i did this with the \v flag to reduce the size. pretty fast, and accurate
There was a problem hiding this comment.
this is the first step in a sed js parser haha
| if g:jsx_pragma_required && !b:jsx_pragma_found | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') | return 0 | endif | ||
| if g:jsx_ext_required && !exists('b:jsx_ext_found') && | ||
| \ !(get(g:,'jsx_check_react_import') && search(s:jsx_prevalent_pattern, 'nw')) |
There was a problem hiding this comment.
Oh, that's right—I forgot this function was setting the filetype on .js files only, not files in general.
|
Can you rebase this on master? I'm having trouble upstreaming the change. |
|
what about github.com ? says it is clean and mergeable |
|
should be fine now |
This uses the vim-jsx plugin to add JSX filetype support, also by setting g:jsx_ext_required to zero it enables JSX in all *.js files. It's not ideal but hopefully the plugin's ftdetect will get better at recognizing React patterns (i.e. when [1] gets merged). [1]: mxw/vim-jsx#130
3 participants
maybe it should be an option though