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

@/CivicSignalBlog - Use aliases in Payload CMS #902

Merged
merged 17 commits into from
Oct 1, 2024

Conversation

m453h
Copy link
Contributor

@m453h m453h commented Sep 27, 2024

Description

This pull request includes several changes to the civicsignalblog application, primarily focusing on using Path aliases for cleaner imports.

Additionally, it fixes failing build-next due to ESLint that were noted after merging #901.

While trying to use path aliases, it was noted that Payload does not resolve the paths as expected, this is dicussed here.

Thus, to resolve this we have used Subpath Import Aliases with custom conditions to ensure that paths are correctly resolved when we run the application in both prod and dev mode.

This has been achieved by:

  • Adding the imports field to the package.json and setting the alias for the src folder to #civicsignalblog. It should be noted that as per the documentation of sub path imports (Entries in the "imports" field must always start with # to ensure they are disambiguated from external package specifiers.)

  • Making the following changes in tsconfig.json and tsconfig.server.json:

    • "module": "esnext", to "module": "NodeNext"
    • "moduleResolution": "node" to "moduleResolution": "NodeNext"
      These changes have been made because subpath imports need a moduleResolution of Node16 or NodeNext.
  • Adding the same alias #civicsignalblog i.e. in webpack resolve > alias config payload.config.ts

  • Using custom conditions in package.json which allows already built code to be imported from dist/ instead of src/ . We have used two conditions where dev resolves to src/ and default that resolves to dist/ thus updated the dev script with --conditions=dev

  • Adding eslint-import-resolver-typescript plugin for enabling ESLint to resolve import statements.


This has been done as an alternative solution that used two packages tsconfig-paths and tsc alias which involved the following steps:

  • Added tsconfig-paths library and updated the dev script with -r tsconfig-paths/register enabling ts-node to correctly resolve path aliases defined in tsconfig.json
  • Added a custom function in tsconfigPathToWebpackAlias.ts that create path aliases that are used in payload.config.ts
  • Added tsc-alias and updated the build-next script to include tsc-alias -p tsconfig.server.json that replaces alias paths with relative paths after typescript compilation.

P.S
The PR might have many modified files but they are mainly adding aliases, one caveat with the current implementation is explained here where we are forced to explicitly include the filename in our imports even for cases where we have an index.js thus in some cases you will see that we are using

import slug from "#civicsignalblog/payload/fields/slug/index";

Instead of

import slug from "#civicsignalblog/payload/fields/slug";

An issue that closely relates to this limitation (though it mainly discusses about exports) can be seen here.


Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@m453h m453h added the chore A task that needs to be done (neither enhancement or bug) label Sep 27, 2024
@m453h m453h self-assigned this Sep 27, 2024
@m453h m453h marked this pull request as ready for review September 27, 2024 08:38
@kilemensi
Copy link
Member

Just saw this custom condition approach that doesn't require extra dependencies. Did you review it / won't it work for us? We should be on TS 5.4 already.

@m453h
Copy link
Contributor Author

m453h commented Sep 27, 2024

Just saw this custom condition approach that doesn't require extra dependencies. Did you review it / won't it work for us? We should be on TS 5.4 already.

Hmm interesting... I hadn't looked at this one...let me quickly test it and get back to you

Copy link
Contributor

@kelvinkipruto kelvinkipruto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG

Copy link
Contributor

@koechkevin koechkevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@m453h Looks good to me, I may have missed something but why are we using #civicsignalblog/* as opposed to @/civicsignalblog/* that we've been using all along

@m453h
Copy link
Contributor Author

m453h commented Sep 30, 2024

@m453h Looks good to me, I may have missed something but why are we using #civicsignalblog/* as opposed to @/civicsignalblog/* that we've been using all along

@koechkevin , we have to use "#civicsignalblog/*" because we are using Subpath Imports we have no option but to use # since "Entries in the "imports" field must always start with # to ensure they are disambiguated from external package specifiers."

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏽

--

Can you update the PR description to match this new direction we've taken?

@m453h
Copy link
Contributor Author

m453h commented Sep 30, 2024

👍🏽

--

Can you update the PR description to match this new direction we've taken?

Sure, I was planning to do that...I just need to resolve one minor issue with the imports, currently we cant implicitly import from folders (i.e. the index.js file doesn't get resolved automatically thus we are are forced to import such files by explicitly appending index in our import statements

e.g. in createPagesCollection.js we are importing fullTiltle and slug as:

import fullTitle from "#civicsignalblog/payload/fields/fullTitle/index";
import slug from "#civicsignalblog/payload/fields/slug/index";

@m453h m453h requested a review from koechkevin October 1, 2024 07:56
@m453h
Copy link
Contributor Author

m453h commented Oct 1, 2024

@CodeForAfrica/tech is this PR a go or a no-go? 👀 🤞

@koechkevin
Copy link
Contributor

@CodeForAfrica/tech is this PR a go or a no-go? 👀 🤞

kaende

Copy link
Member

@kilemensi kilemensi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kilemensi
Copy link
Member

@koechkevin

Screenshot 2024-10-01 at 15 28 36

@m453h m453h added this pull request to the merge queue Oct 1, 2024
Merged via the queue into main with commit 06f77d6 Oct 1, 2024
4 checks passed
@m453h m453h deleted the chore/civicsignalblog-use-aliases branch October 1, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore A task that needs to be done (neither enhancement or bug)
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

4 participants