Skip to content

Commit

Permalink
exclude /node_modules/ from _watch by default (#1794)
Browse files Browse the repository at this point in the history
  • Loading branch information
EslamHiko authored and hiroppy committed Apr 29, 2019
1 parent 7f3b8c9 commit 4c52153
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 1 deletion.
10 changes: 9 additions & 1 deletion lib/Server.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,15 @@ class Server {

this.sockets = [];

this.watchOptions = options.watchOptions || {};
if (!options.watchOptions) {
options.watchOptions = {};
}
// ignoring node_modules folder by default
options.watchOptions.ignored = options.watchOptions.ignored || [
/node_modules/,
];
this.watchOptions = options.watchOptions;

this.contentBaseWatchers = [];
// Replace leading and trailing slashes to normalize path
this.sockPath = `/${
Expand Down
75 changes: 75 additions & 0 deletions test/ContentBase.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,81 @@ describe('ContentBase', () => {
}, 1000);
});
});

describe('test ignoring node_modules folder by Default', () => {
jest.setTimeout(30000);

beforeAll((done) => {
server = helper.start(config, {
contentBase: contentBasePublic,
watchContentBase: true,
});
// making sure that chokidar has read all the files
server.contentBaseWatchers[0].on('ready', () => {
done();
});
req = request(server.app);
});

afterAll((done) => {
helper.close(() => {
done();
});
});

it('Should ignore node_modules & watch bar', (done) => {
const watchedPaths = server.contentBaseWatchers[0].getWatched();
// check if node_modules folder is not in watched list
const folderWatched = !!watchedPaths[
path.join(contentBasePublic, 'node_modules')
];
expect(folderWatched).toEqual(false);
// check if bar folder is in watched list
expect(watchedPaths[path.join(contentBasePublic, 'bar')]).toEqual([
'index.html',
]);

done();
});
});

describe('test not ignoring node_modules folder', () => {
jest.setTimeout(30000);

beforeAll((done) => {
server = helper.start(config, {
contentBase: contentBasePublic,
watchContentBase: true,
watchOptions: {
ignored: /bar/,
},
});
// making sure that chokidar has read all the files
server.contentBaseWatchers[0].on('ready', () => {
done();
});
req = request(server.app);
});

afterAll((done) => {
helper.close(() => {
done();
});
});

it('Should watch node_modules & ignore bar', (done) => {
const watchedPaths = server.contentBaseWatchers[0].getWatched();
// check if node_modules folder is in watched list
expect(
watchedPaths[path.join(contentBasePublic, 'node_modules')]
).toEqual(['index.html']);
// check if bar folder is not in watched list
const folderWatched = !!watchedPaths[path.join(contentBasePublic, 'bar')];
expect(folderWatched).toEqual(false);
done();
});
});

describe('test listing files in folders without index.html using the option serveIndex:false', () => {
beforeAll((done) => {
server = helper.start(
Expand Down
Empty file.

4 comments on commit 4c52153

@blieque
Copy link

@blieque blieque commented on 4c52153 Jun 5, 2019

Choose a reason for hiding this comment

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

Re: lib/Server.js, line 121+.

I can understand the decision to ignore changes in node_modules/ by default, but this could cause problems for Lerna users who have resolve.symlinks set to false. I need this setting for Vue.js and TypeScript to play nicely. Resolving symlinks seems, to me, like an odd thing for webpack to do to begin with, but that's another topic.

The point is, I have common component and style packages under node_modules/ (via npm link), which will appear as such to the dev server with symlinks: false. This new configuration will therefore ignore any changes in those common packages.

Does this new default bring a significant performance gain? I don't object to the change, but it took me a day to find this little commit. Some documentation should probably be brought up to date. I can do that, but I don't know how tightly coupled devServer and webpack-dev-server are allowed to be. Should this default perhaps live in webpack, and therefore be passed to any potential development server (webpack-serve? 🙃).

I now have:

devServer: {
  // ...
  watchOptions: {
    // Ignore changes in packages in `node_modules/`, unless the package is in the @<company>
    // namespace.
    ignored: path => path.includes('/node_modules/') && !path.includes('@<company>'),
  },
},

@hiroppy
Copy link
Member

@hiroppy hiroppy commented on 4c52153 Jun 5, 2019

Choose a reason for hiding this comment

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

@blieque thank you for your feedback. Please submit the monorepo.

@blieque
Copy link

@blieque blieque commented on 4c52153 Jun 5, 2019

Choose a reason for hiding this comment

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

@hiroppy Thanks for the quick reply! Sorry, this is in an internal repository. The configuration I posted does work though. I can set up a demo repository if that would help you, but this will take a little while.

I think the ideal solution may be for webpack to always resolve symlinks when performing this check regardless of resolve.symlinks.

@alexander-akait
Copy link
Member

Choose a reason for hiding this comment

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

Let's speak about this here #1934

Please sign in to comment.