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

Paragon's 'dist' build is not full compiled to JavaScript, won't run without a bundler #3322

Open
bradenmacdonald opened this issue Dec 6, 2024 · 3 comments
Labels
bug Report of or fix for something that isn't working as intended

Comments

@bradenmacdonald
Copy link
Contributor

bradenmacdonald commented Dec 6, 2024

I tried to use Paragon in an XBlock that's using Rollup to transpile its .tsx source code + dependencies into a single .js file. However, I ran into these problems:

 [!] RollupError: node_modules/@openedx/paragon/scss/core/_exports.module.scss (6:17): Expected ';', '}' or <eof>
 (Note that you need plugins to import files that are not JavaScript)
 [!] RollupError: node_modules/@openedx/paragon/dist/Avatar/default-avatar.svg (1:0): Expression expected
 (Note that you need plugins to import files that are not JavaScript)
 [!] RollupError: node_modules/@openedx/paragon/dist/Card/fallback-default.png (1:0): Unexpected character '�'
 (Note that you need plugins to import files that are not JavaScript)

It turns out that even in our dist directory, the JS code is trying to import things that are not JavaScript. For example:

In paragon/dist/utils/breakpoints.js:

import breakpointSizes from '../../scss/core/_exports.module.scss';

In paragon/dist/Avatar/index.js:

import defaultAvatar from './default-avatar.svg';

In paragon/dist/Card/CardImageCap.js:

import cardSrcFallbackImg from './fallback-default.png';

Discussion

Breakpoints:

It seems like the breakpoint sizes themselves (sm, md, lg, xl, xxl) are hard-coded, but the values are customizable, and the intent of the code in breakpoints.js is to read those pixel width values from SCSS variables so they can be customized. If we want to allow people to customize the breakpoints using SCSS variables, and have those available to JavaScript, we should be using CSS variables instead of SCSS variables. Then the JS can just check the CSS variables, and doesn't have to import SCSS at all. This is not only cleaner, but should make the build faster.

However, perhaps the design tokens work already makes this problem irrelevant?

Avatar SVG:

This just seems like a build configuration setting. Our build process should be able to inline the SVG and bundle it into the final .js file. We already do that for the icon SVGs, for example. Not sure why that's not happening.

Card Image PNG:

We shouldn't be including PNG images in the project at all IMHO. The image in question is just a grey rectangle so I think it should be easily replaced by an SVG or even no image at all. If we must include a PNG, it should be a tiny, highly-optimized data: URL.

Other dist optimizations

Please see my vaguely related PR #3284 which is waiting for review + approval :)

@bradenmacdonald bradenmacdonald added the bug Report of or fix for something that isn't working as intended label Dec 6, 2024
@brian-smith-tcril
Copy link
Contributor

re: Card Image PNG

I put a draft PR up moving the png into a data uri. This feels like a non-breaking quick fix option, but of the proposed options I'd much prefer the "even no image at all" one.

@PKulkoRaccoonGang
Copy link
Contributor

PKulkoRaccoonGang commented Dec 9, 2024

It seems like the breakpoint sizes themselves (sm, md, lg, xl, xxl) are hard-coded, but the values are customizable, and the intent of the code in breakpoints.js is to read those pixel width values from SCSS variables so they can be customized. If we want to allow people to customize the breakpoints using SCSS variables, and have those available to JavaScript, we should be using CSS variables instead of SCSS variables. Then the JS can just check the CSS variables, and doesn't have to import SCSS at all. This is not only cleaner, but should make the build faster.

However, perhaps the design tokens work already makes this problem irrelevant?

I think this problem is relevant. At the moment we have a separate token for each breakpoint, these tokens are provided to consumers in the form of custom media breakpoints. But, they are not related to the JavaScript breakpoints from breakpoints.js. We still use the SCSS map with values.

I agree with you. Using CSS variables to provide breakpoint values ​​in JavaScript is definitely a good idea.
Maybe we want to get CSS variable values ​​for breakpoints.js in a way that doesn't import a third-party SCSS file and with values ​​that come from design tokens/CSS variables. Something like this:

const rootStyles = getComputedStyle(document.documentElement);
const gridBreakpointMd = rootStyles.getPropertyValue('--grid-breakpoint-md').trim();

console.log('Grid Breakpoint MD:', gridBreakpointMd);
...
>>> Grid Breakpoint MD: 768px

@bradenmacdonald
Copy link
Contributor Author

@PKulkoRaccoonGang That's what I was thinking, yep. Seems like a sensible approach to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Report of or fix for something that isn't working as intended
Projects
None yet
Development

No branches or pull requests

3 participants