New mainfields option

[keithamus]

Closes #180

/cc @kkalass @ncoden @TrySound

[kkalass]

why do you prepend the field and not append it? Doesn't this line mean that - if a user sets browser, module, jsnext and main to true, she will receive an array [main, jsnext, module, browser]? And doesn't that in turn mean that the precedence order of old lines 140-146 is now reversed? Or did you reverse that order on purpose?

[keithamus]

This is a good point! Thanks for the catch! I'll refactor and add tests to confirm this tomorrow.

[allex]

thanks for mainField support. and i'v fixed the fields order issues at https://github.com/fedorio/rollup-plugin-node-resolve/blame/b63feb54ba226681fecbf690936b10f0dcb521f5/src/index.js#L89-L99

	let mainFields = options.mainFields || [];
	const fields = { 'browser': 0, 'module': 0, 'jsnext': 'jsnext:main', 'main': 0 };
	const keys = Object.keys(options).filter(k => fields.hasOwnProperty(k));


	// build mainFields by plugins options (keep options order)
	mainFields = keys.reduce((list, k) => deprecatedMainField(options, k, list, fields[k] || k), mainFields);


	// set defaults mainFields if none options
	[ 'module', 'main' ].forEach(k => {
		if (!mainFields.includes(k) && options[k] !== false) mainFields.push(k);
	});

[brettz9]

This looks very handy to have... It might end up needed in some cases to add per-module overrides (e.g., I have a project needing browser enabled for several imports, but want to exclude browser in favor of module in one particular one), but this would be great for now.

[keithamus]

Just added a commit to switch the order so browser takes top precedence. I think this is ready to go!

[gregmagolan]

This feature would help with a fix in the bazel rollup_bundle rule. Is there anything left blocking it from getting merged in? Anything I could do to help? Thanks

[gregmagolan]

Ping. @lukastaegert @allex @kkalass Do you have time to review this? Or could we land #429 instead if this is still blocked?

[filipesilva]

I can help finish this too if people don't have time to finish. If @keithamus is ok with it I can take the existing commits on a new PR and fix the merge conflicts. Would that work for you?

[keithamus]

Sorry I hadn't realised this branch had gone stale. It's been rebased now. @lukastaegert I'm still happy with this change, and consider it to be ready to go. Would you kindly review+merge if you're happy with it too?

[Globegitter]

Another gentle ping for @lukastaegert would be great to get this in.

[lukastaegert]

I hope I find time in the next days, for now I am patching up things elsewhere.

[lukastaegert]

Would be nice to have a look why the tests are failing

[lukastaegert]

Ok, started checking this. I am not really happy about how browser is handled. browser CAN be an entry point, but it can also just replace some files. Currently, the "replacing" will happen if browser is "somewhere in the list". This looks really confusing. I would prefer to keep browser separate in the options.

[lukastaegert]

I have fixed the remaining issues and updated the branch. I kept the behaviour for "browser" but also kept the "browser" option as a non-deprecated alternative and extended the Readme. From my side, this is good to go.

[keithamus]

This LGTM 👍. What's the merge etiquette now? Shall I merge+release or would you like to @lukastaegert? This should be semver minor still, right?

[lukastaegert]

I would actually consider it major due to the new deprecation warnings. Please merge and release at your own discretion!

[eps1lon]

I would actually consider it major due to the new deprecation warnings.

In SemVer it MUST be a MINOR for deprecation warnings. You don't have to do a MAJOR ("MAJOR MAY include MINOR").

[lukastaegert]

I do not see a "must" in that document but rather a recommendation. Personally, I like Facebooks approach with React of doing deprecations over two major versions. But sure, I have no problem with doing it in a minor, this is not Rollup core.

[eps1lon]

@lukastaegert

[...] It MUST be incremented if any public API functionality is marked as deprecated [...]

-- https://semver.org/#spec-item-7

The anchor doesn't seem to work

I like Facebooks approach with React of doing deprecations over two major versions

In strict mode you get them between minors: https://github.com/facebook/react/blob/master/CHANGELOG.md#react-1