-
Notifications
You must be signed in to change notification settings - Fork 358
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: detect internal functions created after dev server already started #6257
Conversation
this has the side-effect that we're *always* starting the function server!
depends on netlify/edge-bundler#548 for handling the detected import maps correctly. |
looking at that test failure 🤔 |
I can't reproduce this type check failure locally 🤔 |
Interesting - the CI is pretending that this PR is already merged into main, and running the tests on that state. I could replicate it locally after merging in main. Should be fixed now! |
Hmm, it looks like there's a test that times out on windows. Looks like i'll need to fire up my VM! |
Well well, I can't reproduce it locally on windows. I've restarted CI, hopefully it's "just" flaky? |
Nope, can't replicate locally 😢 |
While debugging via SSH, I was able to narrow it down to one test, and I have a hunch that the test is fixed by 5f05300. The problem was that we weren't awaiting the expectation, so the exception was thrown outside of the test execution. As to why this only occurs now, and only on Windows, I don't have a convincing answer - maybe the changes made the stars align subtly so that the event loop now notices the uncaught rejection. |
Welp, scratch that - the test still fails. But at least it fails equally on all operating systems now. Still can't reproduce locally, though :/ |
Tests pass, Gotcha! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great just a small comment about a type where I think it should stay optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🚀
in #6244 we've merged a change from build that clears the
.netlify/functions-internal
and.netlify/edge-functions
folders on dev startup. We need to update some of our code & tests to handle that, and this PR does it!The main thing is that right now, we're checking if a
.netlify/functions-internal
directory exists once on startup. If it doesn't, we don't watch it, and we don't get notified about functions being added to it.Instead, we should always watch the directory, so any functions added after startup are detected correctly.
This PR fixes all skipped tests one-by-one. It's a rather big refactor, best reviewed commit by commit!