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

[Good First Issue] Handle absolute paths for alternate inject source #211

Open
nmn opened this issue Dec 19, 2023 · 5 comments · May be fixed by #222
Open

[Good First Issue] Handle absolute paths for alternate inject source #211

nmn opened this issue Dec 19, 2023 · 5 comments · May be fixed by #222
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@nmn
Copy link
Contributor

nmn commented Dec 19, 2023

A relatively easy task to get started

For anyone looking to make a contribution to the core Babel plugin, this is a small self-contained task.

Background

By default, when runtimeInjection is true, the function inject styles at runtime is imported as the default import from '@stylexjs/stylex/lib/stylex-inject'.

A recent change has added support for string and objects to be used as the value of runtimeInjection as well.

  • A string is used as the alternate import source for the inject function.
  • An object of the form {from: string, as: string} can be used for a named import from a custom source.

Current behaviour

Today, any custom import source for the inject function is used as a literal string. This doesn't work well when relative file paths are needed which will be different in each file in a project.

Expected Change

The StateManager class should be updated to detect relative or absolute paths and automatically generate the relative paths necessary from the file being transformed.

Here are the steps involved:

  1. If the file is a relative file path. (Starts with ./ or ../)
    • Convert to an absolute file path by path.join with the rootDir passed in.
  2. Update the get runtimeInjection() getter function in state-manager.js to automatically convert any absolute file paths to relative file paths from the current file.
    • NOTE: Absolute file paths start with / on Unix systems, but may start with C:\\ on Windows.

Stretch Goal: The same fix should also be made for importSources.

The importSources option can also accept custom import paths. However, in this case, existing imports need to be verified as coming from a given location.

@nmn nmn added enhancement New feature or request good first issue Good for newcomers labels Dec 19, 2023
@nmn nmn changed the title Handle absolute paths for alternate inject source [Good First Issue] Handle absolute paths for alternate inject source Dec 19, 2023
@purohitdheeraj
Copy link
Contributor

Hi i would like to work on this

@necolas
Copy link
Contributor

necolas commented Dec 20, 2023

What's the use case for allowing custom runtime injection functions?

@purohitdheeraj purohitdheeraj linked a pull request Dec 20, 2023 that will close this issue
2 tasks
@nmn
Copy link
Contributor Author

nmn commented Dec 20, 2023

@necolas This is for consistency with the importSources config option. I didn't actually imagine a custom function, just a custom import path. E.g., if stylex is an indirect dependency and another package is forwarding the StyleX implementations.

@necolas
Copy link
Contributor

necolas commented Dec 20, 2023

Shouldn't the compiler-inserted-code always point to the stylex package's inject module though? The importSources is for source code imports rather than code inserted by the compiler

@nmn
Copy link
Contributor Author

nmn commented Dec 21, 2023

@purohitdheeraj Please don't spend too much time on this while we figure out if this task is redundant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants