Skip to content

feat: enable hmr by default for new projects #4411

New issue

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

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

Already on GitHub? # to your account

Merged
merged 4 commits into from
Mar 13, 2019
Merged

Conversation

Fatme
Copy link
Contributor

@Fatme Fatme commented Mar 1, 2019

Implements: #4394

PR Checklist

@ghost ghost assigned Fatme Mar 1, 2019
@ghost ghost added the new PR label Mar 1, 2019
@cla-bot cla-bot bot added the cla: yes label Mar 1, 2019
Copy link
Contributor

@rosen-vladimirov rosen-vladimirov left a comment

Choose a reason for hiding this comment

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

I think the PR message and the variable isHmrEnabledByDefault are incorrect - this PR enables setting hmr in nsconfig.json and there's nothing like default here. Maybe the variable should be isHmrEnabledInProject, but it is also important to make it easy for end-users to enable the feature in nsconfig.json, i.e. the property should be self explaining what exactly it will do when you set it to true/false

this.argv.hmr = false;
}

if (noBundle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a warning for the user that we will override both hmr and bundle options.

@rosen-vladimirov rosen-vladimirov dismissed their stale review March 12, 2019 11:30

Dismiss as the required changes had been implemented

@Fatme Fatme merged commit d453726 into master Mar 13, 2019
@Fatme Fatme deleted the fatme/hmr-by-default branch March 13, 2019 06:46
@ghost ghost removed the new PR label Mar 13, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants