Skip to content
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

Fix ERR_UNSUPPORTED_DIR_IMPORT for gulpfile.mjs #217

Closed
wants to merge 3 commits into from

Conversation

ngdangtu-vn
Copy link

@ngdangtu-vn ngdangtu-vn commented Aug 19, 2020

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.

  • It should not break anything -> Tested with mocha testcases (available in this repo)
  • 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
Copy link
Author

ngdangtu-vn commented Aug 19, 2020

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
Copy link
Author

ngdangtu-vn commented Aug 21, 2020

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.

@phated
Copy link
Member

phated commented Oct 14, 2020

@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.

@ngdangtu-vn
Copy link
Author

ngdangtu-vn commented Oct 15, 2020

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.

@phated
Copy link
Member

phated commented Jan 7, 2024

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

@ngdangtu-vn
Copy link
Author

@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?

@phated
Copy link
Member

phated commented Jan 14, 2024

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

@phated phated closed this Jan 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants