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

Expose contents of @sveltejs/app-utils as CJS as well as ESM #592

Closed
antony opened this issue Mar 23, 2021 · 2 comments
Closed

Expose contents of @sveltejs/app-utils as CJS as well as ESM #592

antony opened this issue Mar 23, 2021 · 2 comments
Labels
adapters - general Support for functionality general to all adapters

Comments

@antony
Copy link
Member

antony commented Mar 23, 2021

Opening this for feedback before I go ahead and add the build step, but it looks like we can't use commonjs in adapters and then depend on esm modules such as @sveltejs/app-utils if we expect anything to work.

This sort of thing:

const { copy } = require('@sveltejs/app-utils/files');

relies on the exports map present in @sveltejs/app-utils:

"exports": {
		"./files": {
			"import": "./files/index.js"
		},
		"./http": {
			"import": "./http/index.js"
		}
	},

However this needs to be modified to allow require to access these exports too:

"exports": {
		"./files": {
			"import": "./files/index.js",
                        "require": "./files/index.js"
		},
		...
	},

This then causes the following error:

Must use import to load ES Module: /home/ant/Projects/kezia.ws/adapter/node_modules/@sveltejs/app-utils/files/index.js
require() of ES modules is not supported.
require() of /home/ant/Projects/kezia.ws/adapter/node_modules/@sveltejs/app-utils/files/index.js from /home/ant/Projects/kezia.ws/adapter/index.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which defines all .js files in that package scope as ES modules.
Instead rename /home/ant/Projects/kezia.ws/adapter/node_modules/@sveltejs/app-utils/files/index.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /home/ant/Projects/kezia.ws/adapter/node_modules/@sveltejs/app-utils/package.json.

So it looks like we need to build app-utils for cjs and esm in order that it can be used with both types of module.

Happy to go ahead and do this and then fix the vercel and begin adapters accordingly if I'm not missing something.

@antony antony added adapters - general Support for functionality general to all adapters pkg:app-utils labels Mar 23, 2021
@Rich-Harris
Copy link
Member

An alternative is to put those utils into the builder object that gets passed to adapt? The app-utils package has turned out to be a bit of an odd one — app-utils/files is only ever depended on by the adapter itself, while app-utils/http is only ever depended on by the adapter's output (where it's fine for it to be ESM, because that stuff gets bundled. The whole thing just feels a bit messy and ripe for refactoring.

Based on how things are right now, if we had a builder.copy(from, to) method we could get rid of the copy helper. Since adapters already include @sveltejs/kit as a devDependency so that they can import the types, the get_body helper (which is used in adapter-node and adapter-vercel, as well as Kit itself) could live in Kit. Voila, no more app-utils.

@antony
Copy link
Member Author

antony commented Mar 23, 2021

Right, this makes a lot of sense. In that case that's probably the refactor I need to do! I'll have a crack later unless somebody beats me to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
adapters - general Support for functionality general to all adapters
Projects
None yet
Development

No branches or pull requests

2 participants