gulp-cli icon indicating copy to clipboard operation
gulp-cli copied to clipboard

Fix ERR_UNSUPPORTED_DIR_IMPORT for gulpfile.mjs

Open ngdangtu-vn opened this issue 5 years ago • 4 comments

What is this PR for?

This is a ERR_UNSUPPORTED_DIR_IMPORT fixed patch. (And did a bit code refactor in require-or-import.js file)

Before this patch, gulp-cli won't be able to read directory URL for ESM mode. For example:

/gulpfile.js		# work
/gulpfile.js/index.js	# work
/gulpfile.mjs		# work
/gulpfile.mjs/index.mjs	# fail

The only solution is append index.mjs to the directory. And I fixed it.

Was it tested yet?

Almost everything.

  • [x] It should not break anything -> Tested with mocha testcases (available in this repo)
  • [x] It should be able to read gulpfile.mjs directory -> Tested directly in my own repo (a)
  • [ ] A mocha testcase for reading gulpfile.mjs directory -> I have no idea how to do it, so this task is under researching

If it possible, I would like to merge this patch to gulp-cli.

ngdangtu-vn avatar Aug 19 '20 11:08 ngdangtu-vn

Import directory testcase mentioned above:

(a) My testcase was simple, I create an index.mjs inside gulpfile.mjs:

// repo structure
/
	.d/
	src/
		gulpfile.mjs/
			index.mjs
	a/
		files.txt
	b/
		x.conf
	c.txt
// gulpfile.mjs/index.mjs file
import gulp from 'gulp'
import path from 'path'

const dir = [
	path.join('..', 'a'),
	path.join('..', 'b/*.conf'),
	path.join('..', 'c.txt')
]

const dest = '../.d/'

export function copy() {
	return gulp.src(dir, { base: '..' })
		.pipe(gulp.dest(dest))
}

export default gulp.series(copy)

From /, I cd src and then gulp copy. The result works as it should be, all the files/dir move to the /.d/.

ngdangtu-vn avatar Aug 19 '20 11:08 ngdangtu-vn

One small problem appeared. If users set { "type": "module" } in package.json, then gulp-cli will always receive ERR_REQUIRE_ESM error because NodeJs will see every scripts in that package as ESMs. It is a feature. Users may have a 2 options with { "type": "module" }:

  1. Disable: use gulpfile.mjs for ESM and gulpfile.js for CJS
  2. Enable: use ESM syntax only in gulpfile. If CJS syntax is used, users may face this error: ReferenceError: require is not defined

On the bright side, if this PR gets refused. We can still organize gulpfile.mjs by enabling { "type": "module" } and use gulpfile.js/index.js with ESM syntax.

ngdangtu-vn avatar Aug 21 '20 11:08 ngdangtu-vn

@ngdangtu-vn Does this change the behavior people expect from the import syntax in node? I don't really want to define our own behavior for that.

phated avatar Oct 14 '20 22:10 phated

Does this change the behavior people expect from the import syntax in node?

Yes and no:

  • Yes because the import of NodeJs couldn't read dir in dir/index.mjs structure.
  • No because this only affect on how CLI load files, and CLI only. It means you what happens inside gulpfile still follows every ESM rules in NodeJs. Moreover, from the first place, I don't think people will put much thought about how CLI load files.

Therefore, this patch is still safely to merge.

ngdangtu-vn avatar Oct 15 '20 12:10 ngdangtu-vn

@ngdangtu-vn I believe this needs conflicts resolved since I merged #239

phated avatar Jan 07 '24 20:01 phated

@phated It has been too long so I don't really remember what I did back then :)) I can resolve this but do we really need this PR anymore?

ngdangtu-vn avatar Jan 09 '24 09:01 ngdangtu-vn

That's fine. I wasn't sure I wanted to include this so we can just close it out.

phated avatar Jan 14 '24 02:01 phated