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

Add cjs config support #968

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Aug 31, 2020

Changes

This PR adds support for using snowpack.config.cjs in addition to all of the existing config formats. This was a blocker for me when trying to use snowpack in a project that had "type": "module" specified.

Turns out all that was required was upgrading cosmiconfig to v7 and 1 line of code change.

One important consideration is that cosmiconfig v7 drops support for node 8, now requiring node 10+: https://github.com/davidtheclark/cosmiconfig/blob/master/CHANGELOG.md#700. Looks like snowpack already requires node version >=10.19.0 in engines, so I imagine this might not need to be considered a breaking change, but calling it out just in case we might want to save this for a major version for whatever reason.

I've also updated the appropriate section in the docs to include the new format. Let me know if there's any other changes you'd like to see.

Testing

I tested this manually by building locally and copy pasting the build to the project with snowpack.config.cjs config, and then manually bumping the version of cosmiconfig in that project. Seems to be working great.

Since this functionality is almost entirely handled by cosmiconfig, I didn't feel a new test here would add much value, but happy to add one if folks disagree.

@lewisl9029 lewisl9029 requested a review from a team as a code owner August 31, 2020 00:24
@vercel
Copy link

vercel bot commented Aug 31, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/camyhpuyh
✅ Preview: https://snowpack-git-fork-lewisl9029-cjs-config-support.pikapkg.vercel.app

@FredKSchott
Copy link
Owner

This looks great! I have a question related to how we can document this: Would the config have worked as .js & ESM, if you had defined your config file as ESM?

// snowpack.config.js
// does this work?
export default {
  ...
}

@lewisl9029
Copy link
Contributor Author

This looks great! I have a question related to how we can document this: Would the config have worked as .js & ESM, if you had defined your config file as ESM?

// snowpack.config.js
// does this work?
export default {
  ...
}

Here's the error I got when I tried this:

Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: ...\snowpack.config.js
require() of ES modules is not supported.

I believe we'd need some special case handling for "type": "module" projects in cosmiconfig to use import instead of require for this to work, which might not be a trivial change (I haven't looked into their source code yet). In the mean time, this feels like a simple short term solution to unblock usage in "type": "module" projects.

Copy link
Owner

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, this feels like a simple short term solution to unblock usage in "type": "module" projects.

+1, thanks for the explanation and extra details

@FredKSchott FredKSchott merged commit 0398fc3 into FredKSchott:master Aug 31, 2020
@lewisl9029 lewisl9029 deleted the cjs-config-support branch September 1, 2020 02:44
@nickolay nickolay mentioned this pull request Jan 7, 2021
8 tasks
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.

2 participants