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

support TypeScript-format config #1253

Merged
merged 9 commits into from
Oct 18, 2020
Merged

support TypeScript-format config #1253

merged 9 commits into from
Oct 18, 2020

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Oct 8, 2020

Changes

Added support for TypeScript-format config, so the config file can be snowpack.config.ts now, which allow config file to be type-checked.

Testing

Added a new "build" test for this change. Snapshots are generated.

Docs

Added mention about supporting TypeScript-format config at "reference" section.

@g-plane g-plane requested a review from a team as a code owner October 8, 2020 15:23
@vercel
Copy link

vercel bot commented Oct 8, 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/hkxfy9xea
✅ Preview: https://snowpack-git-ts-format-config.pikapkg.vercel.app

'snowpack.config.json',
],
loaders: {
Copy link
Owner

Choose a reason for hiding this comment

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

strong +1 on this feature request, but I think this implementation will break any relative imports (../ out of the temp dir is going to point somewhere else).

I'm not sure of a better way to do this though. We could create the '.snowpack.config.cjs' file alongside snowpack.config.ts temporarily, and then delete it when we're done. It sounds like cosmicconfig does some caching, which means this wouldn't have to happen every time. That's the best I can come up with right now

Copy link
Contributor Author

@g-plane g-plane Oct 9, 2020

Choose a reason for hiding this comment

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

Outdated. Please read next comment.

I've re-implemented this part (see latest commit). The new way is using Node.js built-in vm module and execute it. Because the code is wrapped in a function, variables created in the config file won't be leaked into outer scope. And, there won't be filesystem operations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the config file will be bundled by esbuild. That means any imports will be bundled into a single file, so placing the compiled file in temp directory should be safe now.

Copy link
Owner

Choose a reason for hiding this comment

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

That's clever! It's definitely a solution to the relative imports problem but introduces a lot of extra effort with potential for slow down and bugs. I wouldn't feel comfortable unless this also came with a console.warn("TypeScript config support is still experimental").

tbh I still prefer the temporary single-file method outlined in https://github.com/pikapkg/snowpack/pull/1253/files#r501834625 to bundling the entire config. A light touch for a workaround like this is valuable. And, since cosmicconfig does caching, the users should rarely notice it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we can put the temporary file into current working directory, I think bundling is necessary, because this allows to import other TS files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

still thinking about this. St the very least, we want to get the current release that's on master out first, and then revisit this post-release. Hopefully getting that out tonight/tomorrow.

@FredKSchott
Copy link
Owner

FredKSchott commented Oct 8, 2020

Also, tests were red on master overnight. Rebasing on master should get tests passing.
Also for the merge conflict that you're seeing, "docs/docs/10-reference.md" is now at "docs/10-reference.md"

@FredKSchott
Copy link
Owner

Okay, I've thought about this some more and am +1 on merging, with the caveat of adding a special error around the bundling and require-ing of a TS config file. Basically, something to the effect of:

try {
  const ... = esbuild.build()
  const ... = require()
  return ...
} catch (err) {
  logger.error('Warning: TypeScript config file support is still experimental. Consider moving to JavaScript if you continue to have problems');
  throw err;
}

@g-plane thanks again for your patience here! If you can rebase this PR and add this fix, I'll +1 and merge in time for the next v2.15 release.

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.

LGTM! Had to remove the docs mention for now since this won't go out immediately (and our docs site does release immediately) but thank you for adding that.

@FredKSchott FredKSchott merged commit 5b0866b into FredKSchott:master Oct 18, 2020
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