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

vendor path-to-regexp #126

Merged
merged 2 commits into from
Sep 3, 2024
Merged

vendor path-to-regexp #126

merged 2 commits into from
Sep 3, 2024

Conversation

tbeseda
Copy link
Contributor

@tbeseda tbeseda commented Sep 3, 2024

This branch now vendors path-to-regexp to avoid Arc hydrate grabbing the latest when a consumer of Enhance deploys.

See below for historical context.

previous discussion

Don't merge! Pretty sure bad things will happen... 😨

At the moment when Arc hydrate treeshakes the Enhance any-catchall route, there is not package.json so it will install the latest of any discovered package.

This has worked fine because most dependencies here are stable. However, there's a rewrite of path-to-regexp in a major version, v8.0.0.
The new version is picked up by hydrate this route blows up since the API changed.

You can replicate this locally:

  1. any Enhance app
  2. run npx @architect/architect hydrate (this happens on deploy to AWS or Begin)
  3. see that hydrate does work on node_modules/@enhance/arc-plugin-enhance/src/http/any-catchall/
  4. navigate to that dir + /node_modules/path-to-regexp/package.json
  5. it's v8.0.0
  6. that's bad, Enhance wants v6.2-ish

I could use the updated v8 of path-to-regexp, but that doesn't really fix the problem.

Instead, I think we should give hydrate a package manifest to work with.

However, I think we've done this before. I believe this would then mean that route would not get dependencies (via treeshaking; which is disabled when a manifest is present) for userland code in node_modules/@architect/views. But I can't recall.

Another option is to change how hydrate works for plugin-provided Lambdae source directories. It could reference plugin-space package.json for the version. But with @architect/views (which is app/) it does a merge? idk could be messy.

Also relevant: #87

Let's discuss.

In the meantime I will update our router implementation to use the new path-to-regexp

@tbeseda tbeseda marked this pull request as ready for review September 3, 2024 17:09
@tbeseda tbeseda changed the title add package.json to catchall vendor path-to-regexp Sep 3, 2024
@tbeseda tbeseda merged commit eed174d into main Sep 3, 2024
21 checks passed
@tbeseda tbeseda deleted the anycatchall-package_json branch September 3, 2024 17:16
@github-actions github-actions bot locked and limited conversation to collaborators Sep 3, 2024
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.

1 participant