Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

added support for including node_module folders #358

Closed
wants to merge 3 commits into from
Closed

added support for including node_module folders #358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fd96e5
added support for including node_module folders
stephenkiers Jul 5, 2018
a4e5078
fix tests
stephenkiers Jul 5, 2018
d365a35
process @zept folder
stephenkiers Jul 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,17 @@ cd my-app/
npm start
```

## Node_module packages that also need typescript processing
To enable typescript processing on node_module folders (npm packages) you may add the following environment variable to your package.
```sh
REACT_APP_TYPESCRIPT_NODE_MODULES_FOLDERS="$$folder name(s) space delimited$$"
```
Ex. for internal, namespaced packages like `{root}/node_modules/@company-name/unique` you could set
```sh
REACT_APP_TYPESCRIPT_NODE_MODULES_FOLDERS="@company-name"
```


Copy link
Collaborator

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. 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.

Copy link
Author

@stephenkiers stephenkiers Jul 10, 2018 •
edited
Loading

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.

## Migration

In general, most upgrades won't require any migration steps to work, but if you experience problems after an upgrade, please file an issue, and we'll add it to the list of migration steps below.
Expand Down
14 changes: 11 additions & 3 deletions packages/react-scripts/config/paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ const getPublicUrl = appPackageJson =>
// like /todos/42/static/js/bundle.7289d.js. We have to know the root.
function getServedPath(appPackageJson) {
const publicUrl = getPublicUrl(appPackageJson);
const servedUrl = envPublicUrl ||
(publicUrl ? url.parse(publicUrl).pathname : '/');
const servedUrl =
envPublicUrl || (publicUrl ? url.parse(publicUrl).pathname : '/');
return ensureSlash(servedUrl, true);
}

Expand All @@ -68,6 +68,12 @@ module.exports = {
// @remove-on-eject-begin
const resolveOwn = relativePath => path.resolve(__dirname, '..', relativePath);

// create array of node_module folders that also need typescript processing
const folders = process.env.REACT_APP_TYPESCRIPT_NODE_MODULES_FOLDERS;
const typescriptNodeModules = !folders
? []
: folders.split(' ').map(folder => resolveApp(`node_modules/${folder}`));

// config before eject: we're in ./node_modules/react-scripts/config/
module.exports = {
dotenv: resolveApp('.env'),
Expand All @@ -90,11 +96,13 @@ module.exports = {
// These properties only exist before ejecting:
ownPath: resolveOwn('.'),
ownNodeModules: resolveOwn('node_modules'), // This is empty on npm 3
typescriptModules: typescriptNodeModules,
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment for README.md.

};

const ownPackageJson = require('../package.json');
const reactScriptsPath = resolveApp(`node_modules/${ownPackageJson.name}`);
const reactScriptsLinked = fs.existsSync(reactScriptsPath) &&
const reactScriptsLinked =
fs.existsSync(reactScriptsPath) &&
fs.lstatSync(reactScriptsPath).isSymbolicLink();

// config before publish: we're in ./packages/react-scripts/config/
Expand Down
2 changes: 1 addition & 1 deletion packages/react-scripts/config/webpack.config.dev.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ module.exports = {
// Compile .tsx?
{
test: /\.(ts|tsx)$/,
include: paths.appSrc,
include: [paths.appSrc, ...(paths.typescriptModules || [])],
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment for README.md.

use: [
{
loader: require.resolve('ts-loader'),
Expand Down
2 changes: 1 addition & 1 deletion packages/react-scripts/config/webpack.config.prod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ module.exports = {
// Compile .tsx?
{
test: /\.(ts|tsx)$/,
include: paths.appSrc,
include: [paths.appSrc, ...(paths.typescriptModules || [])],
Copy link
Collaborator

Choose a reason for hiding this comment

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

See comment for README.md.

use: [
{
loader: require.resolve('ts-loader'),
Expand Down
2 changes: 1 addition & 1 deletion packages/react-scripts/scripts/utils/createJestConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ module.exports = (resolve, rootDir, isEjecting) => {
),
},
transformIgnorePatterns: [
'[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs|ts|tsx)$',
'[/\\\\]node_modules[/\\\\](?!@zept).+\\.(js|jsx|mjs|ts|tsx)$',
Copy link
Collaborator

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 webpack rule for tsx? files covers the node_modules folder, those files should not be listed on this ignore list.

Copy link
Author

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.

],
moduleNameMapper: {
'^react-native$': 'react-native-web',
Expand Down