Skip to content
This repository has been archived by the owner on May 22, 2024. It is now read-only.

fix(exports parser): handle as default export #1658

Closed
wants to merge 1 commit into from
Closed

fix(exports parser): handle as default export #1658

wants to merge 1 commit into from

Conversation

lilnasy
Copy link

@lilnasy lilnasy commented Nov 19, 2023

Summary

image
  • This resulted in runtimeAPIVersion failing to be correctly detected as v2 here
  • And an incorrect version being picked by the builder here
  • The CLI then goes on to deploy critically misbehaving functions, like in this case
image

The implementation may be done better, I don't have the full context of the codebase. Feel free to improve upon it.

@lilnasy lilnasy requested a review from a team as a code owner November 19, 2023 03:49
@Skn0tt
Copy link
Contributor

Skn0tt commented Nov 20, 2023

Hey @lilnasy, thanks for opening this PR - much appreciated! Coincidentally, I opened #1657 on friday which might address the same thing. I'll compare & review them sometime today.

@Skn0tt
Copy link
Contributor

Skn0tt commented Nov 20, 2023

Yup, your fix is pretty much the same as mine from Friday :D I've added the code from your screenshot as a test case to 7dfba88. Going to close your PR, and merge mine - will ping here again once it's deployed.

@Skn0tt Skn0tt closed this Nov 20, 2023
@lilnasy lilnasy deleted the patch-1 branch November 20, 2023 21:18
@lilnasy
Copy link
Author

lilnasy commented Nov 20, 2023

@Skn0tt Thanks for that! Do you guys have stats on how many users are on older versions of netlify-cli?
I would probably have to workaround the issue with regex. Is there a way to explicitly inform the cli of the runtime version?

@Skn0tt
Copy link
Contributor

Skn0tt commented Nov 21, 2023

will ping here again once it's deployed.

The update went out to the CLI in https://github.com/netlify/cli/releases/tag/v17.5.3 and was deployed to our CI build system a couple minutes ago.

Is there a way to explicitly inform the cli of the runtime version?

I'm not aware of a way, nor did I find something in the code :/ This might be something we could build, will look into it.

Do you guys have stats on how many users are on older versions of netlify-cli?

Just ran the numbers for you. It seems like in the past 24hrs of netlify deploy executions, only about 1/3 of them were on recent versions (17.5.x), and others were lagging behind quite a bit. But it should be pretty straight-forward for most to update their CLI version.

If the regex is straight-forward for you, then that might be the best bet. If it isn't, maybe you could try to detect the version of the Netlify CLI that's being used to build, and warn/error on versions below 17.5.3? Let me know if you want to go that way, happy to help in how to detect the CLI version.

@lilnasy
Copy link
Author

lilnasy commented Nov 21, 2023

It would be relatively reliable and easy to string replace. I will do that.

This might be something we could build, will look into it

Yes, it will help in a similar situation in the future.

@Skn0tt
Copy link
Contributor

Skn0tt commented Nov 23, 2023

Yes, it will help in a similar situation in the future.

Opened an internal issue about it, let's see when we get to it 🤷

@Skn0tt
Copy link
Contributor

Skn0tt commented Jan 8, 2024

@lilnasy it's now possible to detect the version of the Netlify CLI that's running using the NETLIFY_CLI_VERSION environment variable: netlify/cli#6226

Skn0tt added a commit to netlify/build that referenced this pull request May 21, 2024
…1657)

* fix: detect alt version of remix handler

* chore: add another test for astro-style config

Test case taken from netlify/zip-it-and-ship-it#1658.

Co-Authored-By: @lilnasy

* Update tests/unit/runtimes/node/in_source_config.test.ts

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>

---------

Co-authored-by: Eduardo Bouças <mail@eduardoboucas.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants