-
Notifications
You must be signed in to change notification settings - Fork 326
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
Fixing the issue with aliased imports in Nextjs #202
Conversation
Hi @rayan1810! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on! This is 90% right already.
Some things I want to consider:
- I want to look at how Typescript accepts aliases to ensure we support all the same patterns
- There will need to be some kind of validation of the
aliases
object. And we should probably store the regular expressions in theoptions
object up-front. - The function to actually resolve the paths can be simpler
- The output path currently starts with
./
, we will need topath.join
that with therootDir
path.
return regex; | ||
} | ||
// a function that resolves the path of files with aliases | ||
function aliasPathResolver(importPath: string = '', aliases: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can be simpler IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You don't need a regEx at all.
- There are two possible cases:
- The
alias
ends with*
- If so, check if the importedPath starts with the alias (minus the
*
) - If it does, replace the
alias
with the replacement at the beginning of the string
- If so, check if the importedPath starts with the alias (minus the
- The
alias
does not end with*
- Check if the imported path is equal to the alias
- If so, replace it with the alias replacement
- The
- If the imported path was replaced by an alias,
path.join
it with therootDir
option to get an absolute file path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmn so I looked into this a little more and see that it doesn't make any sense if alias does not end with *
. A case that is possible is something like this: "@util/*": ["./src/util/*/src/index.ts"]
making this @util/someDir
correspond to ./src/util/someDir/src/index.ts
. The current regex logic also doesn't match this. I'm fixing with this in mind now. Please let me know if you have any use case other than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been studying how the aliases work in tsconfig
and package.json
and here's what I found.
I'll use the term key
and value
to refer to the import path configurations.
Common Patterns
- Aliases can contain 0 or 1
*
characters. Never More. - The
*
character is NOT a glob pattern, but rather equivalent to the regex/.*/
and will match any string. - If the
key
has a*
then the should also contain a single*
. This patterns means that the string in the position of the*
in thekey
should be inserted into the*
in the value to get the final path. - The
*
can exist anywhere other than the beginning of thekey
.
Specific TS patters
- When multiple
key
s match, Typescript use the one with the longest string before the*
. - The
value
can be an array of strings. These strings are tried in order as they may or may not exist.
Specific package.json
patterns
- All
key
s must start with the#
character, but not#/
. E.g.#src/*
is OK. - The
value
s must be single strings. No arrays. - The first matching
key
is used, not the longest.
TS is adopting package.json
from TS 5.4
In the next version, Typescript will automatically pick up the "import path" configuration from package.json
. The option to manually configure tsconfig
will also remain.
What a simple implementation can look like.
Since there is at most a single *
character in the paths, there is no need for RegEx.
Assuming there are no arrays, the code could looks something like this:
function getAliasedPath(pathConfig, importPath) {
for (const [key, value] of Object.entries(pathConfig)) {
if (key.includes('*')) {
const [before, after] = key.split('*');
if (importPath.startsWith(before) && importPath.endsWith(after)) {
const wildcard = importPath.slice(
before.length,
after.length > 0 ? -after.length : undefined
);
return value.split('*').join(wildcard);
}
} else if (key === importPath) {
return value;
}
}
return importPath;
}
This code can be extended a bit for the case where value is an Array to return the first path that actually exists, but I'm happy to skip that for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nmn, this was really helpful. Also addressed all the changes requested previously.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know when you consider the PR ready again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmn you can review this, I've also updated the docs api reference as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a look and it looks good. I'll come back to this in a few days. As I'm gonna be taking a break for New Years!
* Fix: Validate options passed into the Babel plugin * Add full validation for options with good errors
* Docs: Fix code example typo (facebook#290) * Handle `keyframe` within `defineVars` * Add test * Format * Fix grammar * Update stylex-validation-define-vars-test.js --------- Co-authored-by: k14lb3 <[email protected]>
* Add eslint-plugin package * Add initial test suite * Improve CSS handling and tests * Address some of the comments * Add babel scripts for jsx, flow and ts * Add package diff * Add snapshots and fix nits * Fix rename bug * Add esbuild example app * Fix incorrect loaders * Update package-lock.json * Remove unnecessary file * Fix some nits in example * Add additional style to text fixture * Remove ext from import in example * Address some of the comments * Update package-lock.json * Add missing function call and snapshot * Remove comment * Pluralize function name * Add types * Fix build script * Add docs * Fix tabulation * Retake snapshots * Format prettier * Add more missing types * Run formatter * Fix formatting * Lock dependency and fix import * Disable unused key * Add docs * Update package-lock.json * Update package-lock.json * Add snapshots * Address comments
@nmn is it possible to run the checks again the type check has failed because of some issue with the npm install connection error, might be an issue with github. |
@@ -75,6 +77,63 @@ type StyleXStateOptions = $ReadOnly<{ | |||
... | |||
}>; | |||
|
|||
function validateAliases(aliases: any): StyleXOptions['aliases'] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding any
type since options has been defined as object of string: mixed, causing issues with the aliases which should be type object but are getting assigned mixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not break any typing since this is just a validation function and returns the currect types for the aliases object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use $FlowFixMe
instead of any
. Other than that, I'll add validation for this. I'm also looking into using zod
or something similar instead of the hand-rolled solution I have right now.
I'm looking for something with good error messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure @nmn, I've updated the typing for now. I'll also check into zod implementation of yours and try to add validation using that while you are looking into it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmn Try Effect Schema. It's from the author of io-ts (Zod was based on that). Schema is faster and better designed.
https://github.com/Effect-TS/effect/tree/main/packages/schema
@rayan1810 Sorry for the churn, but could you rebase this again? I'm getting ready to merge this. |
@nmn do you want me to remove the validation function from the aliases for now? |
@rayan1810 Yeah, I can add validation after merging using the same framework as I'm using otherwise. Remember to rebase onto the |
@nmn I've resolved the conflicts and removed the validation function. Had to add |
@rayan1810 Thanks! I'll take from here. |
* feat: alias path resolution in nextjs using config * chore: simplify the aliases in nextjs example app * feat: docs update for aliases config in babel plugin and nextjs plugin --------- Co-authored-by: k14lb3 <[email protected]>
* feat: alias path resolution in nextjs using config * chore: simplify the aliases in nextjs example app * feat: docs update for aliases config in babel plugin and nextjs plugin --------- Co-authored-by: k14lb3 <[email protected]>
* feat: alias path resolution in nextjs using config * chore: simplify the aliases in nextjs example app * feat: docs update for aliases config in babel plugin and nextjs plugin --------- Co-authored-by: k14lb3 <[email protected]>
What changed / motivation ?
Added changes in StateManager class in babel plugin, introduced an extra step of resolution if aliases are provided in babel and nextjs config. This fixes some part of this issue #40
Linked PR/Issues
[Fixes #40 ]
Pre-flight checklist
Contribution Guidelines