Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(webpackDevServer): Add watchOptions for webpackDevServer #1814

Merged
merged 1 commit into from
Sep 26, 2016

Conversation

JSMike
Copy link
Contributor

@JSMike JSMike commented Aug 23, 2016

Add watchOptions to webpackDevServerConfiguration to conditionally enable polling option in watchpack
Remove additional blank lines from end of serve-watchpack.ts so that only one is remaining

@JSMike
Copy link
Contributor Author

JSMike commented Aug 23, 2016

In cases where Angular CLI is running in a shared directory on linux VM on a windows host the webpack dev server isn't detecting file changes from the host environment. (ex: Whenever a docker dev environment is running on a windows host.)
This is solved by adding poll option to the webpack dev server configuration. By default when no poll interval is set watchpack will use native file system change detection. This change allows for setting the poll interval which will turn on polling.

With this change the following can be added to the defaults object in angular-cli.json to change the default polling option:

  "poll": 1000

When the poll option isn't included in angular-cli.json it will be set to the default, which is undefined, and native file system change detection will be used.

Here is the link to the webpack docks regarding watchOptions.poll:
https://webpack.github.io/docs/configuration.html#watchoptions-poll

Note: Also removed 4 out of the 5 blank lines at the end of the file that I'm assuming shouldn't be there.

Edit: Update change required for angular-cli.json

@JSMike
Copy link
Contributor Author

JSMike commented Aug 23, 2016

Minor edit. instead of making all watchOptions configurable only make poll configurable. This will make configuration changes to angular-cli.json cleaner by only having to set a poll flag or interval

@JSMike
Copy link
Contributor Author

JSMike commented Aug 24, 2016

rebased to master

@JSMike
Copy link
Contributor Author

JSMike commented Aug 24, 2016

restarting travis

@JSMike JSMike closed this Aug 24, 2016
@JSMike JSMike reopened this Aug 24, 2016
@JSMike
Copy link
Contributor Author

JSMike commented Aug 24, 2016

Hi All,

The recent changes to config broke the change I made. I'm looking into it.

@JSMike JSMike force-pushed the watchOptions branch 2 times, most recently from 64904fc to b9ad5e1 Compare August 24, 2016 22:47
@hansl
Copy link
Contributor

hansl commented Aug 25, 2016

You need to update the schema JSON, not the d.ts. We're talking internally as to whether or not this change is wanted.

@JSMike
Copy link
Contributor Author

JSMike commented Aug 25, 2016

@hansl should it be in both or just the schema?

As stated previously, without the change there's no direct way to have webpack detect changes on shared directories between linux/windows via a VM such as in docker development environments. With the common "poll" flag option in defaults it can be applied to other addons which may want to conditionally add polling for detecting file changes.

Please let me know if anything else is needed.

@JSMike
Copy link
Contributor Author

JSMike commented Aug 25, 2016

Another note on polling, this may also resolve the watch issues for users developing in Window's 10's Ubuntu Usermode, I haven't tested that myself though. Reference

@JSMike JSMike force-pushed the watchOptions branch 2 times, most recently from bd0fbff to 350b667 Compare August 25, 2016 18:05
@@ -138,6 +138,9 @@
},
"lazyRoutePrefix": {
"type": "string"
},
"poll": {
"type": [ "boolean", "number" ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This ends up causing a warning in SchemaClassFactory._createChildProperty when doing an ng serve because the function isn't built to handle a field that can be of more than one type. The example "type": [ "boolean", "number" ] is valid JSON Schema per http:https://jsonschemalint.com/draft5/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed that in a163186 the custom-typings.d.ts file defined poll as only being a number. I've updated all of my poll definitions to reflect this.

@JSMike JSMike force-pushed the watchOptions branch 4 times, most recently from 369f016 to 828ef20 Compare September 1, 2016 14:12
@JSMike
Copy link
Contributor Author

JSMike commented Sep 1, 2016

I've updated this PR to reflect the typing decision made in a163186 to define the poll as only being a number and not also being boolean.

@JSMike JSMike force-pushed the watchOptions branch 2 times, most recently from 8ed0c62 to 5b301df Compare September 7, 2016 04:24
@simb
Copy link

simb commented Sep 12, 2016

Having this will also allow docker users to setup the toolchain and support the watched building. I'd love to see this get merged.

@JSMike JSMike force-pushed the watchOptions branch 2 times, most recently from 7bf6734 to cc16467 Compare September 15, 2016 04:17
JSMike added a commit to JSMike/angular-cli that referenced this pull request Sep 29, 2016
With angular#1814 the `defaults.poll` option was added to the cli config. This setting won't be applied to the karma defaults without this update.
JJayet pushed a commit to JJayet/angular-cli that referenced this pull request Oct 2, 2016
…r#1814)

Add watchOptions to webpackDevServerConfiguration to conditionally enable
polling option in watchpack
Remove additional blank lines from end of serve-watchpack.ts so that only
one is remaining
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 3, 2016
With angular#1814 the `defaults.poll` option was added to the cli config. This setting won't be applied to the karma defaults without this update.
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 3, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting is currently set to apply to `ng serve` but not `ng test`. This fix adds the setting to the defaults that are applied to karma's config object, so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 3, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting is currently set to apply to `ng serve` but not `ng test`. This fix adds the setting to the defaults that are applied to karma's config object, so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
deebloo pushed a commit to deebloo/angular-cli that referenced this pull request Oct 3, 2016
…r#1814)

Add watchOptions to webpackDevServerConfiguration to conditionally enable
polling option in watchpack
Remove additional blank lines from end of serve-watchpack.ts so that only
one is remaining
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 5, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting is currently set to apply to `ng serve` but not `ng test`. This fix adds the setting to the defaults that are applied to karma's config object, so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.

Fixup
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 5, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting is currently set to apply to `ng serve` but not `ng test`. This fix adds the setting to the defaults that are applied to karma's config object, so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.

Fixup
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 5, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting is currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 5, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 6, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JSMike added a commit to JSMike/angular-cli that referenced this pull request Oct 10, 2016
With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
filipesilva pushed a commit that referenced this pull request Oct 10, 2016
With #1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JJayet pushed a commit to JJayet/angular-cli that referenced this pull request Oct 14, 2016
…r#1814)

Add watchOptions to webpackDevServerConfiguration to conditionally enable
polling option in watchpack
Remove additional blank lines from end of serve-watchpack.ts so that only
one is remaining
JJayet pushed a commit to JJayet/angular-cli that referenced this pull request Oct 14, 2016
…ar#2486)

With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
JJayet pushed a commit to JJayet/angular-cli that referenced this pull request Oct 14, 2016
…r#1814)

Add watchOptions to webpackDevServerConfiguration to conditionally enable
polling option in watchpack
Remove additional blank lines from end of serve-watchpack.ts so that only
one is remaining
Brocco pushed a commit that referenced this pull request Oct 19, 2016
With #1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
texel pushed a commit to splice/angular-cli that referenced this pull request Nov 3, 2016
…ar#2486)

With angular#1814 the `defaults.poll` property was added to angular-cli.json. This configuration setting currently applies to `ng serve` but not `ng test`. This fix adds the poll value to the karma defaults so that if you set `defaults: { poll: 1000 }` it will apply to both `ng serve` and `ng test`.
@monad98
Copy link

monad98 commented Dec 22, 2016

Without documentation, I don't know how to ignore "node_modules" directory. Can anyone tell me how to?
https://webpack.js.org/configuration/watch/#watchoptions-ignored

@JSMike
Copy link
Contributor Author

JSMike commented Dec 22, 2016

@monad98 Currently there's no configuration options for this, the cli's webpack config would need to be updated to allow for setting this. https://github.com/angular/angular-cli/blob/master/packages/angular-cli/tasks/serve-webpack.ts#L100 and the other files from this PR to update the config schema.

A separate issue/PR should be created for this to examine if this is desired. By ignoring files here the bundle would not rebuild when files are changed, if new modules are added or if existing node_modules are updated IMO it should trigger a rebuild. I understand the expense of watching the entire node_modules directory structure for changes on some older machines, but I haven't personally seen much of a slowdown here. @hansl will need to weigh in on if this is something that's wanted.

@grantlucas
Copy link

Apologies for digging up this older PR but it's the first one with a comment that's my exact issue.

@monad98

Without documentation, I don't know how to ignore node_modules directory. Can anyone tell me how to? https://webpack.js.org/configuration/watch/#watchoptions-ignored


I'm running a Vagrant setup for a few things and consistently, if I use poll for webpack, I need to exclude node_modules or else it maxes out the CPU and over time will crash the VM as it eats up RAM.

When it comes to ng build, if I put --poll=1000 it sits at max CPU usage. Excluding node_modules has been the solution to this in the past for other Webpack setups. Personally, if node_modules changes, it'd be reasonable to restart the ng serve to ensure it's all set up properly.

@monad98 Were you ever able to resolve ignoring node_modules?

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants