- Notifications
You must be signed in to change notification settings - Fork488
added support for including node_module folders#358
Uh oh!
There was an error while loading.Please reload this page.
Conversation
DorianGrey left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd say this is a bit too complicated. Importingts source files fromnode packages is more of an exceptional than regular behavior. Extending theinclude clause of thetsx? rule to cover thenode_modules folder is more simple and does not hurt anyone not using it, since it is only applied in this very specific case.
stephenkiersJul 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment forREADME.md.
| { | ||
| test:/\.(ts|tsx)$/, | ||
| include:paths.appSrc, | ||
| include:[paths.appSrc, ...(paths.typescriptModules||[])], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment forREADME.md.
| { | ||
| test:/\.(ts|tsx)$/, | ||
| include:paths.appSrc, | ||
| include:[paths.appSrc, ...(paths.typescriptModules||[])], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
See comment forREADME.md.
| }, | ||
| transformIgnorePatterns:[ | ||
| '[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|ts|tsx)$', | ||
| '[/\\\\]node_modules[/\\\\](?!@zept).+\\.(js|jsx|mjs|ts|tsx)$', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
I tested by changing the files directly in node_module folder until I was able to get myrepository 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!