added support for including node_module folders #358
Conversation
DorianGrey
left a comment
DorianGrey
left a comment
There was a problem hiding this comment.
Sorry for the delay - was a bit more busy the last days.
Review attached.
| REACT_APP_TYPESCRIPT_NODE_MODULES_FOLDERS="@company-name" | ||
| ``` | ||
|
|
||
|
|
There was a problem hiding this comment.
I'd say this is a bit too complicated. Importing ts source files from node packages is more of an exceptional than regular behavior. Extending the include clause of the tsx? rule to cover the node_modules folder is more simple and does not hurt anyone not using it, since it is only applied in this very specific case.
There was a problem hiding this comment.
Are you sure? I feel like that would slow down a large project during dev builds/testing to process every package...
I think what may be best is us maintaining a fork for ourselves, and maybe I will write a medium article on what I changed for anyone else that wants to do the same; since it is a pretty limited use-case.
I don't want to decrease dev performance for all users of this package just to meet our niche requirement.
| // These properties only exist before ejecting: | ||
| ownPath: resolveOwn('.'), | ||
| ownNodeModules: resolveOwn('node_modules'), // This is empty on npm 3 | ||
| typescriptModules: typescriptNodeModules, |
There was a problem hiding this comment.
See comment for README.md.
| { | ||
| test: /\.(ts|tsx)$/, | ||
| include: paths.appSrc, | ||
| include: [paths.appSrc, ...(paths.typescriptModules || [])], |
There was a problem hiding this comment.
See comment for README.md.
| { | ||
| test: /\.(ts|tsx)$/, | ||
| include: paths.appSrc, | ||
| include: [paths.appSrc, ...(paths.typescriptModules || [])], |
There was a problem hiding this comment.
See comment for README.md.
| }, | ||
| transformIgnorePatterns: [ | ||
| '[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|ts|tsx)$', | ||
| '[/\\\\]node_modules[/\\\\](?!@zept).+\\.(js|jsx|mjs|ts|tsx)$', |
There was a problem hiding this comment.
Two issue hier:
- What exactly is the
@zept, and why is listed here? - If the
webpackrule fortsx?files covers thenode_modulesfolder, those files should not be listed on this ignore list.
There was a problem hiding this comment.
This is a mistake in that I updated the package for our internal use to also ensure tests processed the package as well. I feel including all packages in the transform would slow tests down too.
I am voting we close this PR as to niche.
2 participants
I tested by changing the files directly in node_module folder until I was able to get my repository working properly.
I am hoping this is an acceptable extension of the project. If not, let me know and I will simply use my own repo for my project.
thanks!