feat(node-resolve): Export defaults

[MichaelDeBoey]

Rollup Plugin Name: node-resolve

This PR contains:

• feature

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

Note from @shellscape: We're going to change this to a named export and will break. Needs a new major.

List any relevant issue numbers:

Description

Closes #299

[shellscape]

Hey thanks for the PR! Looks like that broke a few things. Give that a test run locally.

[MichaelDeBoey]

Hmmm tests are failing because I'm doing an export default and a named export now.
But that shouldn't fail, right? 🤔

@shellscape Any idea on how to make this a non-breaking change? 🤔

[NotWoods]

We might need some wrapper for the common JS output.

Before:

module.exports = nodeResolve(...)

After:

exports.default = nodeResolve(...)
exports.defaults = defaults

Ideal:

mod = nodeResolve(...)
mod.defaults = defaults
module.exports = mod

---

Also, since export default and export defaults are quite similar, I think defaultOptions or DEFAULT_OPTIONS would be better names.

[MichaelDeBoey]

@NotWoods Wrapping the module still fails tests

[shellscape]

@MichaelDeBoey please merge from master again. we fixed up some CI related things today.

[MichaelDeBoey]

@shellscape Rebased, but CI is still failing tho 😕

[shellscape]

Bummer. Next step is to run tests locally to triage from there: cd packages/node-resolve && pnpm run test

[MichaelDeBoey]

@shellscape It's still the same error

 TypeError {
    message: 'nodeResolve is not a function',
  }

This is because the plugin now exports both a default and a named export.
Wrapping it like @NotWoods suggested isn't working either.

So I don't think we can do this without a breaking change/major release. 🤔

[NotWoods]

The wrapper should be extracted to a separate file, and used as the entry point for only the CJS build. The presence of an export const and export default statement is throwing off Rollup's CJS build.

[MichaelDeBoey]

@NotWoods Could you point me in the right direction on how I could best fix this? 🤔

[NotWoods]

This behavior might be better inside an output plugin, but here's something I whipped together:

// cjs-wrapper.js
import nodeResolve, { DEFAULT_OPTIONS } from './index';

nodeResolve.DEFAULT_OPTIONS = DEFAULT_OPTIONS;

export default nodeResolve;

// rollup.config.js
import babel from 'rollup-plugin-babel';
import json from '@rollup/plugin-json';

import pkg from './package.json';

const plugins = [
    json(),
    babel({
      presets: [
        [
          '@babel/preset-env',
          {
            targets: {
              node: 6
            }
          }
        ]
      ]
    })
];
const external = Object.keys(pkg.dependencies).concat(['fs', 'path', 'os', 'util']);

export default [
	{
		input: 'src/index.js',
		plugins,
		external,
		output: [
    		{ file: pkg.module, format: 'es' }
  		]
	},
	{
		input: 'src/cjs-wrapper.js',
		plugins,
		external,
		output: [
    		{ file: pkg.main, format: 'cjs' }
  		]
	},
];

[MichaelDeBoey]

@shellscape @NotWoods All is green now 🙂

[shellscape]

Thanks for working through the issues getting CI good and a solid wrapper going. Now that we've got a good approach, I've got a few points I wanted to touch on. Please don't get discouraged!

[MichaelDeBoey]

@shellscape @NotWoods All is green again now 🙂

[shellscape]

Thanks for continuing to work through this. I'm sorry for the delayed reviews, doing the best I can with the time I've got on my end.

[shellscape]

@MichaelDeBoey I'll fix this conflict when I get the chance to circle back.

[MichaelDeBoey]

@shellscape Rebase onto upstream/master

[shellscape]

Thanks for all the hard work that went into this one.

[MichaelDeBoey]

With pleasure! 🙂