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

feat: automatic create tsconfig for editor support on functions #6036

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukasholzer
Copy link
Collaborator

@lukasholzer lukasholzer commented Oct 5, 2023

Fixes https://github.com/netlify/pod-dev-foundations/issues/595

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes the editor support for the new functions V2 API. By that we enforce that the editor is in sync with the build values


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

📊 Benchmark results

Comparing with 2a68876

  • Dependency count: 1,369 (no change)
  • Package size: 379 MB ⬇️ 0.00% decrease vs. 2a68876

@lukasholzer
Copy link
Collaborator Author

With that configuration, I get even in JavaScript completion for the Netlify.env

CleanShot 2023-10-06 at 10 07 20

@lukasholzer lukasholzer marked this pull request as ready for review October 6, 2023 08:08
@lukasholzer lukasholzer requested a review from a team as a code owner October 6, 2023 08:08
@lukasholzer lukasholzer force-pushed the feat/automatic-create-tsconfig-for-functions-v2-api branch from 6355b77 to 631e53c Compare October 16, 2023 12:40
// "extends": "../tsconfig.json", /** If you want to share configuration enable the extends property (like strict: true) */
"compilerOptions": {
"noEmit": true /** This tsconfig.json is only used for type checking and editor support */,
"module": "ESNext",
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to set this or moduleResolution anymore? I think all we need is allowImportingTsExtensions + noEmit for the .ts extensions, and then checkJs + allowJs for the JavaScript files?

*/
export async function checkTsconfigForV2Api(config) {
// if no functionsDir is specified or the dir does not exist just return
if (!config.functionsDir || !existsSync(config.functionsDir)) {
Copy link
Member

Choose a reason for hiding this comment

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

Have you considered moving this logic into the FunctionsRegistry (like we did with

this.checkTypesPackage()
), so that we have this kind of logic centralised in one place? It also means that you don't have to recreate any logic for checking whether we have functions or not, because the registry already knows that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure but then we can't run it on the create command.


try {
const require = createRequire(config.functionsDir)
require.resolve('@netlify/functions')
Copy link
Member

Choose a reason for hiding this comment

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

I stole this from you and it's already been shipped 😬

this.checkTypesPackage()

@sarahetter
Copy link
Contributor

@lukasholzer is this PR still relevant, or should we close/draft?

@sarahetter sarahetter removed the request for review from a team April 24, 2024 16:08
@lukasholzer
Copy link
Collaborator Author

@sarahetter yes but sadly never had time to finish it up

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.

4 participants