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

fix for compiling with multiple deps in content config #88

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mayel
Copy link

@mayel mayel commented Oct 9, 2024

Solves a compilation error when trying to put this in config:

content: [
  swifti: [
    "lib/**/*swiftui*",
    {:my_custom_lib, "lib/**/*swiftui*"},
    {:other_lib, [
      "lib/**/*swiftui*",
      "priv/**/*swiftui*"
    ]}
  ]
],
output: "priv/static/assets/"

@bcardarella
Copy link
Contributor

Can you add a test for this, we use config/test.exs for this so you just need to add a list to an additional tuple form here: https://github.com/liveview-native/live_view_native_stylesheet/blob/main/config/test.exs#L13

@mayel
Copy link
Author

mayel commented Oct 9, 2024

Sure thing, though I'd have to add something like {:another_library, "lib/**/*.*"} to encounter the same issue (since this has to do with compilation order and which deps are already compiled when compiling a module). I tested and tests pass when including that in config after this change FWIW.

@mayel
Copy link
Author

mayel commented Oct 9, 2024

I should note that the stylesheet module is also in a dependency itself in our case.

@mayel
Copy link
Author

mayel commented Oct 9, 2024

Added the config

@bcardarella
Copy link
Contributor

@mayel can you give me a bit more background in that case, what exactly is failing that requires this change?

@mayel
Copy link
Author

mayel commented Oct 9, 2024

Mix.Project.deps_paths only includes already compiled dependencies, so Path.join was trying to join on nil and crashing

@bcardarella
Copy link
Contributor

Oh I see, this is a compilation order issue then. So in that case your PR just ignores the error it doesn't fix the underlying issue of discovering those paths.

@mayel
Copy link
Author

mayel commented Oct 9, 2024

Couldn't we just provide a list of patterns, relative to the CWD, instead of specifying otp_aps though?

@bcardarella
Copy link
Contributor

Couldn't we just provide a list of patterns, relative to the CWD, instead of specifying otp_aps though?

In your case you could if you'd like, you can simply just provide a list of relative paths

@bcardarella
Copy link
Contributor

@mayel can you confirm that the relative paths worked for your need? I don't think this needs to be merged if so

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.

2 participants