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

exts option should replace the defaults #94

Closed
nlwillia opened this Issue Apr 12, 2017 · 13 comments

Comments

6 participants
@nlwillia

nlwillia commented Apr 12, 2017

v0.6.2

As mentioned after #89 was closed, an array of extensions passed with the exts option gets appended to the default list instead of replacing it as you would expect. If you specifically don't want to watch one of the default extensions (ex: js), there's no way to override it currently.

@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Apr 12, 2017

Owner

How should this be implemented? Let's discuss before a PR.

Owner

napcs commented Apr 12, 2017

How should this be implemented? Let's discuss before a PR.

@nlwillia

This comment has been minimized.

Show comment
Hide comment
@nlwillia

nlwillia Apr 12, 2017

I see 3 possible use cases for the exts setting. Right now, only 1 and 2 are possible.

  1. You want to use the defaults as provided.
  2. You want to add to the defaults.
  3. You want to completely replace the defaults.

Changing exts to replace would be a breaking change for current instances of case 2. It would still be possible to do it, but you would have to fully specify the default list, or maybe reference a provided static property. (ex: livereload.createServer({exts: livereload.defaultExts.concat(['foo'])}))

A backwards-compatible alternative would be to introduce another property, either an array to override the defaults or a flag to switch the handling of exts from concat to replace.

nlwillia commented Apr 12, 2017

I see 3 possible use cases for the exts setting. Right now, only 1 and 2 are possible.

  1. You want to use the defaults as provided.
  2. You want to add to the defaults.
  3. You want to completely replace the defaults.

Changing exts to replace would be a breaking change for current instances of case 2. It would still be possible to do it, but you would have to fully specify the default list, or maybe reference a provided static property. (ex: livereload.createServer({exts: livereload.defaultExts.concat(['foo'])}))

A backwards-compatible alternative would be to introduce another property, either an array to override the defaults or a flag to switch the handling of exts from concat to replace.

@rauschma

This comment has been minimized.

Show comment
Hide comment
@rauschma

rauschma Jul 17, 2017

One possibility – instead of an array, pass a function:

exts: arr => arr.filter(x => x !== 'css'),

rauschma commented Jul 17, 2017

One possibility – instead of an array, pass a function:

exts: arr => arr.filter(x => x !== 'css'),
@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Oct 12, 2017

Owner

The more I think about this, the more I think it's best to break backwards compatibility with this option and implement a new option for the old behavior. exts will overwrite, and we'll have a new flag to append options.

I'll release LiveReload 0.6.3 with a noisy deprecation warning and 0.6.4 will remove the old behavior.

Owner

napcs commented Oct 12, 2017

The more I think about this, the more I think it's best to break backwards compatibility with this option and implement a new option for the old behavior. exts will overwrite, and we'll have a new flag to append options.

I'll release LiveReload 0.6.3 with a noisy deprecation warning and 0.6.4 will remove the old behavior.

@TheLudd

This comment has been minimized.

Show comment
Hide comment
@TheLudd

TheLudd Oct 26, 2017

@napcs
If you intend to make this breaking change you should really release it as 0.7.0.

Or why not 1.0.0? :)

TheLudd commented Oct 26, 2017

@napcs
If you intend to make this breaking change you should really release it as 0.7.0.

Or why not 1.0.0? :)

@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Oct 26, 2017

Owner

Yea really great point. We'll go to 0.7.0 for the next release.

Owner

napcs commented Oct 26, 2017

Yea really great point. We'll go to 0.7.0 for the next release.

@kasbah

This comment has been minimized.

Show comment
Hide comment
@kasbah

kasbah Nov 15, 2017

Hey, thanks very much for your work on this.

I just wanted to point out that your deprecation notice really confused me.

*** DEPRECATION WARNING *** The exts option will REPLACE extensions in 0.6.4. ***

I thought exts was replacing an old option called extensions but wasn't clear why I was getting the warning. Then I thought maybe there is a by missing and exts will be replaced. Anyway, maybe I am dumb but it confused me. I would suggest:

*** DEPRECATION WARNING *** The exts option will REPLACE default extensions in 0.6.4. ***

kasbah commented Nov 15, 2017

Hey, thanks very much for your work on this.

I just wanted to point out that your deprecation notice really confused me.

*** DEPRECATION WARNING *** The exts option will REPLACE extensions in 0.6.4. ***

I thought exts was replacing an old option called extensions but wasn't clear why I was getting the warning. Then I thought maybe there is a by missing and exts will be replaced. Anyway, maybe I am dumb but it confused me. I would suggest:

*** DEPRECATION WARNING *** The exts option will REPLACE default extensions in 0.6.4. ***
@nahtnam

This comment has been minimized.

Show comment
Hide comment
@nahtnam

nahtnam Jan 26, 2018

I'm still really confused. I currently have this:

const liveReloadServer = liveReload.createServer({
  exts: ['marko'],
});

What do I need to do to fix this deprecation? How do I get rid of the message?

nahtnam commented Jan 26, 2018

I'm still really confused. I currently have this:

const liveReloadServer = liveReload.createServer({
  exts: ['marko'],
});

What do I need to do to fix this deprecation? How do I get rid of the message?

@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Jan 27, 2018

Owner

@nahtnam Nothing. The deprecation message exists because the behavior will change in the next version. You will continue to get this message until the next release. See my message above - noisy deprecation warning for 0.6.3.

Owner

napcs commented Jan 27, 2018

@nahtnam Nothing. The deprecation message exists because the behavior will change in the next version. You will continue to get this message until the next release. See my message above - noisy deprecation warning for 0.6.3.

@nahtnam

This comment has been minimized.

Show comment
Hide comment
@nahtnam

nahtnam Jan 27, 2018

Wait, for the same behavior to apply, wouldnt my code have to be:

const liveReloadServer = liveReload.createServer({
  exts: ['html', 'css', 'js', 'png', 'gif', 'jpg', 'php', 'php5', 'py', 'rb', 'erb', 'coffee', 'marko'],
});

The default exts are:

defaultExts = [
  'html', 'css', 'js', 'png', 'gif', 'jpg',
  'php', 'php5', 'py', 'rb', 'erb', 'coffee'
]

nahtnam commented Jan 27, 2018

Wait, for the same behavior to apply, wouldnt my code have to be:

const liveReloadServer = liveReload.createServer({
  exts: ['html', 'css', 'js', 'png', 'gif', 'jpg', 'php', 'php5', 'py', 'rb', 'erb', 'coffee', 'marko'],
});

The default exts are:

defaultExts = [
  'html', 'css', 'js', 'png', 'gif', 'jpg',
  'php', 'php5', 'py', 'rb', 'erb', 'coffee'
]
@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Jan 29, 2018

Owner

@nahtnam in the current version (0.6.3), you would just add "marko". It would add "marko" to the existing default extensions. But this behavior is confusing. So in the next (unreleased) version, the exts option will REPLACE all the default extensions with only what you specify. But that's what the deprecation warning you're seeing is all about - When I release the new version, if you're using the exts option, you might all of a sudden not be watching the files you expect. So we'll add a new option to preserve this behavior.

Owner

napcs commented Jan 29, 2018

@nahtnam in the current version (0.6.3), you would just add "marko". It would add "marko" to the existing default extensions. But this behavior is confusing. So in the next (unreleased) version, the exts option will REPLACE all the default extensions with only what you specify. But that's what the deprecation warning you're seeing is all about - When I release the new version, if you're using the exts option, you might all of a sudden not be watching the files you expect. So we'll add a new option to preserve this behavior.

@nahtnam

This comment has been minimized.

Show comment
Hide comment
@nahtnam

nahtnam Jan 29, 2018

Yeah sorry, I meant that the above code would be what you would need to have the same output with the new version, correct?

nahtnam commented Jan 29, 2018

Yeah sorry, I meant that the above code would be what you would need to have the same output with the new version, correct?

@napcs

This comment has been minimized.

Show comment
Hide comment
@napcs

napcs Feb 24, 2018

Owner

0.7.0 is now released which closes this issue.

The -e or --exts option now replaces all default extensions.
The -e or --extraExts option appends default extensions to what you specify. This is the old behavior.

Owner

napcs commented Feb 24, 2018

0.7.0 is now released which closes this issue.

The -e or --exts option now replaces all default extensions.
The -e or --extraExts option appends default extensions to what you specify. This is the old behavior.

@napcs napcs closed this Feb 24, 2018

QingWei-Li added a commit to docsifyjs/docsify-cli that referenced this issue Jul 19, 2018

fix: fix livereload deprecation warning (#38)
Since livereload v0.6.3, we get the warning:

```
$ docsify serve ./docs --open
*** DEPRECATION WARNING *** The exts option will REPLACE extensions in 0.6.4. ***
```

As the author of livereload [commented](napcs/node-livereload#94 (comment)): in the next version, the exts option will REPLACE all the default extensions with only what you specify. That means `exts: ['md']` will only watch markdown files. If we want to also watch default extensions, we should use `extraExts: ['md']`.

This PR upgrade `livereload` and use `extraExts` option to fix the warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment