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

Automatically annotate file paths if dynamicPaths include JS modules or is a function #1815

Closed
wants to merge 2 commits into from

Conversation

mythmon
Copy link
Member

@mythmon mythmon commented Nov 13, 2024

This is a bit messy, especially the leaky abstraction of the config file, but it would fix the problem.

Some alternate approaches I thought about while writing this:

  • Unconditionally enable file annotations
    • This is simpler, but creates a larger diff and is unneeded on many data apps.
  • Enable annotation on dynamicPath modules and all of their transitive dependencies.
    • This would be a lot harder for me, and I'm not sure if it would be robust enough.
  • Somehow force that file annotations are enabled, possibly by rebuilding JS files, when deploying.
    • Haven't quite figured this one out yet
  • Reject un-annotated builds on the server side if they seem required.

@mythmon mythmon requested review from visnup, Fil and mbostock November 13, 2024 19:47
@Fil
Copy link
Contributor

Fil commented Nov 13, 2024

Unconditionally enable file annotations sounds good to me.

@mbostock
Copy link
Member

Reject un-annotated builds on the server side if they seem required.

I feel like this is the way to go. Even with this PR, old releases of Framework will still not work, so we’d need to enforce that a minimum version of Framework is used. So why not more directly enforce that Framework was built with path annotation enabled? (I don’t recall if we stash some metadata when building, but we certainly could if we aren’t already.)

@mythmon mythmon closed this Nov 14, 2024
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.

3 participants