Skip to content

Clean up deprecation handling - #2076

Merged
lukastaegert merged 4 commits into
masterfrom
config-cleanup
Mar 24, 2018
Merged

Clean up deprecation handling#2076
lukastaegert merged 4 commits into
masterfrom
config-cleanup

Conversation

@lukastaegert

@lukastaegert lukastaegert commented Mar 21, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

This resolves some issues surrounding deprecations and also fixes an outdated warning. Most notably, specifying dest would overwrite any existing output object even if it was created by previous deprecation warnings.

Also, the -d flag has now been deprecated.

@lukastaegert
lukastaegert requested a review from guybedford March 21, 2018 07:11
guybedford
guybedford approved these changes Mar 22, 2018
View reviewed changes

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking good.

Comment thread test/cli/samples/merge-deprecations/rollup.config.js Outdated
format: 'es',
dest: '_actual/bundle1.js',
output: {
file: '_actual/bundle1.js',

@guybedford guybedford Mar 22, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps make this _actual/bundle2.js so that we are testing the precedence here for future changes too.

@lukastaegert lukastaegert Mar 23, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

With the current implementation, dest: bundle1.js would replace output.file: bundle2.js(as it is currently the case for all deprecations, i.e. the deprecated config takes precedence). I guess you would rather expectoutput.file` to win?

I could change that, but then I would try to change it for all deprecations at once. Should I do that?

@guybedford guybedford Mar 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It does seem to make sense that the newer form should have higher precedence.

In theory it's a breaking change, but it could be worthwhile.

@lukastaegert lukastaegert added this to the 0.58.0 milestone Mar 23, 2018
@lukastaegert
* Slightly simplify deprecations
ee20baf 
* Do not let deprecated "dest" option overwrite existing "output"
* Enable using old and new options together, albeit with warnings
* Resolves #2005
* Resolves #2006
@lukastaegert
Deprecate -d option, resolves #2077
a5e18e1
@lukastaegert
Fix test
cc6bf5b
@lukastaegert

lukastaegert commented Mar 24, 2018

Copy link
Copy Markdown
Member Author

@guybedford Updated to not overwrite existing options with deprecated ones.

guybedford
guybedford approved these changes Mar 24, 2018
View reviewed changes

@guybedford guybedford left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to merge.

@lukastaegert
lukastaegert merged commit ffe4e5b into master Mar 24, 2018
@lukastaegert
lukastaegert deleted the config-cleanup branch March 25, 2018 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guybedford guybedford guybedford approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.58.0

2 participants

@lukastaegert @guybedford