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

Mark @rei/cdr-tokens as external #188

Merged
merged 4 commits into from
Nov 7, 2024
Merged

Mark @rei/cdr-tokens as external #188

merged 4 commits into from
Nov 7, 2024

Conversation

HeavyMedl
Copy link
Collaborator

@HeavyMedl HeavyMedl commented Nov 2, 2024

Problem

I see an SSR runtime exception from Nitro (Nuxt) derived from dead references in @rei/cedar's distributed modules. When Nuxt builds the application (nuxt build), it outputs optimized server assets to .output/server, where you'd run a productionized Nuxt app from.

The Nuxt build heuristics exclude nested node_modules directories, instead hoisting them to the top-level .output/server/node_modules. However, some @rei/cedar modules are dependent on those modules existing in a relative location:

// .output/server/node_modules/@rei/cedar/dist/src/components/image/CdrImg.vue2.mjs
import { CdrRadiusSoft as f, CdrRadiusSofter as C, CdrRadiusRound as y } from "../../../node_modules/@rei/cdr-tokens/dist/rei-dot-com/js/cdr-tokens.mjs";

This causes the following Nuxt/SSR runtime exception:

[Vue Router warn]: uncaught error during route navigation:
Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/kmedley/code/alpine-composer/.output/server/node_modules/@rei/cedar/dist/node_modules/@rei/cdr-tokens/dist/rei-dot-com/js/cdr-tokens.mjs' imported from /Users/kmedley/code/alpine-composer/.output/server/node_modules/@rei/cedar/dist/src/components/modal/CdrModal.vue2.mjs
    at finalizeResolution (node:internal/modules/esm/resolve:265:11)
    at moduleResolve (node:internal/modules/esm/resolve:933:10)
    at defaultResolve (node:internal/modules/esm/resolve:1157:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:390:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:359:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:234:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:87:39)
    at link (node:internal/modules/esm/module_job:86:36) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///Users/kmedley/code/alpine-composer/.output/server/node_modules/@rei/cedar/dist/node_modules/@rei/cdr-tokens/dist/rei-dot-com/js/cdr-tokens.mjs'
}

In the context of the Alpine ecosystem, this hasn't been a problem because our processes use a customized SSR setup that copies node_modules into the Crampon SSR sidecar (docker container).

Solution

This PR marks @rei/cdr-tokens as external, so the build system doesn't bundle them into the @rei/cedar distributed modules. Instead, Cedar modules will directly reference @rei/cdr-tokens from node_modules at run-time, which should be the expected behavior.

Side-effects

With @rei/cdr-tokens externalized, we need to adjust module entry points for @rei/cedar, as Vite no longer outputs .dist/src. Those entry points will live in .dist/, thus, we update references in package.json. Additionally, module references to @rei/cdr-tokens are updated to match.

@HeavyMedl HeavyMedl added the bug Something isn't working label Nov 2, 2024
@HeavyMedl HeavyMedl self-assigned this Nov 2, 2024
Copy link
Collaborator Author

@HeavyMedl HeavyMedl left a comment

Choose a reason for hiding this comment

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

Looking at how @rei/cdr-tokens is used, e.g., the JS API for some components uses it directly, means it’s actually a dependency, not a devDep. We want to ensure the lib gets the version of tokens it needs now that it’s (correctly) being treated as an external module.

Copy link
Contributor

@sikhote sikhote left a comment

Choose a reason for hiding this comment

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

Checked within our doc site for kicks and things worked as expected. Not sure why this was not the original method for importing tokens, but this makes sense.

@sikhote
Copy link
Contributor

sikhote commented Nov 4, 2024

@HeavyMedl & @mhewson I can merge/release—any objections or concerns?

@HeavyMedl
Copy link
Collaborator Author

Ship it 🚢

@sikhote sikhote merged commit f5c55cd into main Nov 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants