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

feat: add ParagonWebpackPlugin to support design tokens #546

Merged
merged 23 commits into from
Aug 13, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
452b3e8
feat: expose PARAGON_VERSION as a global variable
adamstankiewicz Jun 5, 2023
eb4fe12
fix: PR feedback
adamstankiewicz Jul 24, 2023
0e3fc56
fix: updates
adamstankiewicz Mar 25, 2024
2e8ee7d
chore: update dependecies in example app
dcoa May 15, 2024
5fa8aff
chore: update add webpack-remonve-empty-scripts and parse5 dependencies
dcoa May 16, 2024
f48437b
fix: install paragon plugins and fix css compiler
dcoa May 16, 2024
e98a4fb
refactor: change paragon package name for openedx
dcoa May 17, 2024
12007ca
refactor: remove runtime config
dcoa May 17, 2024
2bf31f8
revert: example changes
dcoa May 23, 2024
9343528
fix: add a try/catch to config loading in Paragon Plugin to avoid err…
dcoa May 28, 2024
afe8880
refactor: split Paragon Plugin Utils file
dcoa May 29, 2024
3d2f0a1
docs: update JSDoc in config/data/paragonUtils.js
dcoa Jun 5, 2024
d173b8e
refactor: use @openedx/brand scope
dcoa Jun 5, 2024
b7fcfe5
refactor: add utf-8 to readFileSync in paragonUtils
dcoa Jun 17, 2024
73a83f4
perf: replace filter for reduce in getParagonThemeCss
dcoa Jun 17, 2024
dfdcbdb
refactor: change entryPoints and CacheGroups variable definition.
dcoa Jun 17, 2024
b1f8364
docs: add comment to describe the use of RemoveEmptyScriptsPlugin
dcoa Jul 30, 2024
4ac4d42
revert: restore env.development processing un webpack.dev.config.js
dcoa Jul 31, 2024
347e820
refactor: remove process.env references in ParagonWebpackPlugin, no s…
dcoa Aug 2, 2024
c019cc9
fix: use openedx scope for webpack.dev.config
dcoa Aug 2, 2024
182100f
fix: allow processing edx and openedx brand scope
dcoa Aug 4, 2024
516c1bf
refactor: support process.env.PARAGON_THEME_URLS
dcoa Aug 9, 2024
27d0f75
fix: make PARAGON_THEME_URLS be defined only as env
dcoa Aug 10, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ module.exports = {
},
globals: {
newrelic: false,
PARAGON_THEME: false,
},
ignorePatterns: [
'module.config.js',
Expand Down
160 changes: 160 additions & 0 deletions config/data/paragonUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
const path = require('path');
brian-smith-tcril marked this conversation as resolved.
Show resolved Hide resolved
const fs = require('fs');

/**
* Attempts to extract the Paragon version from the `node_modules` of
* the consuming application.
*
* @param {string} dir Path to directory containing `node_modules`.
* @returns {string} Paragon dependency version of the consuming application
*/
function getParagonVersion(dir, { isBrandOverride = false } = {}) {
const npmPackageName = isBrandOverride ? '@openedx/brand' : '@openedx/paragon';
Copy link
Member

Choose a reason for hiding this comment

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

In testing this further with the @edx/brand alias installed in this repo's example MFE app, and exposing the injected PARAGON_THEME global variable within App.jsx, the installed brand package is not installed as this function references @openedx/brand rather than the aliased @edx/brand currently used by MFEs (e.g., frontend-app-learning).

I might recommend supporting the following to remain both forward-looking for the @openedx migration and the currently used @edx/brand:

  • @openedx/paragon
  • @openedx/brand (brand override)
  • @edx/brand (brand override)

const pathToPackageJson = `${dir}/node_modules/${npmPackageName}/package.json`;
if (!fs.existsSync(pathToPackageJson)) {
return undefined;
}
return JSON.parse(fs.readFileSync(pathToPackageJson, 'utf-8')).version;
}

/**
* @typedef {Object} ParagonThemeCssAsset
* @property {string} filePath
* @property {string} entryName
* @property {string} outputChunkName
*/
Comment on lines +31 to +36
Copy link
Member

Choose a reason for hiding this comment

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

[unrelated nit, no action needed] now that frontend-build natively supports TypeScript, we might want to consider migrating these JSDoc type definitions to TypeScript. It would help improve the long-term maintainability of these code paths.

I filed a follow-up GitHub issue (#581) to backlog the task to migrate these JSDOc type definitions to TypeScript for future consideration/prioritization.


/**
* @typedef {Object} ParagonThemeVariantCssAsset
* @property {string} filePath
* @property {string} entryName
* @property {string} outputChunkName
*/

/**
* @typedef {Object} ParagonThemeCss
* @property {ParagonThemeCssAsset} core The metadata about the core Paragon theme CSS
* @property {Object.<string, ParagonThemeVariantCssAsset>} variants A collection of theme variants.
*/

/**
* Attempts to extract the Paragon theme CSS from the locally installed `@openedx/paragon` package.
* @param {string} dir Path to directory containing `node_modules`.
* @param {boolean} isBrandOverride
* @returns {ParagonThemeCss}
*/
function getParagonThemeCss(dir, { isBrandOverride = false } = {}) {
const npmPackageName = isBrandOverride ? '@openedx/brand' : '@openedx/paragon';
Copy link
Member

Choose a reason for hiding this comment

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

In testing this further with the @edx/brand alias installed in this repo's example MFE app, and exposing the injected PARAGON_THEME global variable within App.jsx, the installed brand package is not installed as this function references @openedx/brand rather than the aliased @edx/brand currently used by MFEs (e.g., frontend-app-learning).

I might recommend supporting the following to remain both forward-looking for the @openedx migration and the currently used @edx/brand:

  • @openedx/paragon
  • @openedx/brand (brand override)
  • @edx/brand (brand override)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why @edx/brand is not considered is that we would like to encourage the migration of that alias as much as possible (additional context), that is why is included in the list of changes in the PR description, as you can see in the example app PR

Even the current readme for the repository makes reference to @openedx/brand instead of @edx/brand.

{ moduleName: '@openedx/brand', dir: '../src/brand-openedx' }, // replace with your brand checkout

I consider that we could be more explicit about this change in the MFE Migration Guide instead. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Even the current readme for the repository makes reference to @openedx/brand instead of @edx/brand.

To expand on @brian-smith-tcril's response below regarding keeping parity with the existing @edx/brand scope used today given the non-breaking nature of this release, I believe the README prematurely referenced @openedx/brand as part of this PR given that migration of NPM scope for the brand packages has not yet been prioritized/executed. Looks like the brand package was incorrectly grouped with the scope migrations for other NPM packages (i.e., @openedx/paragon and @openedx/frontend-build).

By having the current README recommend @openedx/brand in the module.config.js within the README would never be resolved as the alias since @openedx/brand is not used in consuming apps, so it's currently incorrect documentation.

const pathToParagonThemeOutput = path.resolve(dir, 'node_modules', npmPackageName, 'dist', 'theme-urls.json');

if (!fs.existsSync(pathToParagonThemeOutput)) {
return undefined;
}
const paragonConfig = JSON.parse(fs.readFileSync(pathToParagonThemeOutput, 'utf-8'));
const {
core: themeCore,
variants: themeVariants,
defaults,
} = paragonConfig?.themeUrls || {};

Comment on lines +65 to +70
Copy link

Choose a reason for hiding this comment

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

should we validate if the values exist before use them?, I mean just to avoid future errors, something like if !themeCore || !themeVariants return undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not necessary because we are using destructuring if any of them is not defined that will return undefined by default.

const pathToCoreCss = path.resolve(dir, 'node_modules', npmPackageName, 'dist', themeCore.paths.minified);
const coreCssExists = fs.existsSync(pathToCoreCss);

const themeVariantResults = Object.entries(themeVariants || {}).reduce((themeVariantAcc, [themeVariant, value]) => {
const themeVariantCssDefault = path.resolve(dir, 'node_modules', npmPackageName, 'dist', value.paths.default);
const themeVariantCssMinified = path.resolve(dir, 'node_modules', npmPackageName, 'dist', value.paths.minified);

if (!fs.existsSync(themeVariantCssDefault) && !fs.existsSync(themeVariantCssMinified)) {
return themeVariantAcc;
}

return ({
...themeVariantAcc,
[themeVariant]: {
filePath: themeVariantCssMinified,
entryName: isBrandOverride ? `brand.theme.variants.${themeVariant}` : `paragon.theme.variants.${themeVariant}`,
outputChunkName: isBrandOverride ? `brand-theme-variants-${themeVariant}` : `paragon-theme-variants-${themeVariant}`,
},
});
}, {});

if (!coreCssExists || themeVariantResults.length === 0) {
return undefined;
}

const coreResult = {
filePath: path.resolve(dir, pathToCoreCss),
entryName: isBrandOverride ? 'brand.theme.core' : 'paragon.theme.core',
outputChunkName: isBrandOverride ? 'brand-theme-core' : 'paragon-theme-core',
};

return {
core: fs.existsSync(pathToCoreCss) ? coreResult : undefined,
variants: themeVariantResults,
defaults,
};
}

/**
* @typedef CacheGroup
* @property {string} type The type of cache group.
* @property {string|function} name The name of the cache group.
* @property {function} chunks A function that returns true if the chunk should be included in the cache group.
* @property {boolean} enforce If true, this cache group will be created even if it conflicts with default cache groups.
*/

/**
* @param {ParagonThemeCss} paragonThemeCss The Paragon theme CSS metadata.
* @returns {Object.<string, CacheGroup>} The cache groups for the Paragon theme CSS.
*/
function getParagonCacheGroups(paragonThemeCss) {
if (!paragonThemeCss) {
return {};
}
const cacheGroups = {
[paragonThemeCss.core.outputChunkName]: {
type: 'css/mini-extract',
name: paragonThemeCss.core.outputChunkName,
chunks: chunk => chunk.name === paragonThemeCss.core.entryName,
enforce: true,
},
};

Object.values(paragonThemeCss.variants).forEach(({ entryName, outputChunkName }) => {
cacheGroups[outputChunkName] = {
type: 'css/mini-extract',
name: outputChunkName,
chunks: chunk => chunk.name === entryName,
enforce: true,
};
});
return cacheGroups;
}

/**
* @param {ParagonThemeCss} paragonThemeCss The Paragon theme CSS metadata.
* @returns {Object.<string, string>} The entry points for the Paragon theme CSS. Example: ```
* {
* "paragon.theme.core": "/path/to/node_modules/@openedx/paragon/dist/core.min.css",
* "paragon.theme.variants.light": "/path/to/node_modules/@openedx/paragon/dist/light.min.css"
* }
* ```
*/
function getParagonEntryPoints(paragonThemeCss) {
if (!paragonThemeCss) {
return {};
}

const entryPoints = { [paragonThemeCss.core.entryName]: path.resolve(process.cwd(), paragonThemeCss.core.filePath) };
Object.values(paragonThemeCss.variants).forEach(({ filePath, entryName }) => {
entryPoints[entryName] = path.resolve(process.cwd(), filePath);
});
return entryPoints;
}

module.exports = {
getParagonVersion,
getParagonThemeCss,
getParagonCacheGroups,
getParagonEntryPoints,
};
35 changes: 35 additions & 0 deletions config/jest/setupTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,38 @@ const testEnvFile = path.resolve(process.cwd(), '.env.test');
if (fs.existsSync(testEnvFile)) {
dotenv.config({ path: testEnvFile });
}

global.PARAGON_THEME = {
paragon: {
version: '1.0.0',
themeUrls: {
core: {
fileName: 'core.min.css',
},
defaults: {
light: 'light',
},
variants: {
light: {
fileName: 'light.min.css',
},
},
},
},
brand: {
version: '1.0.0',
themeUrls: {
core: {
fileName: 'core.min.css',
},
defaults: {
light: 'light',
},
variants: {
light: {
fileName: 'light.min.css',
},
},
},
},
};
44 changes: 44 additions & 0 deletions config/webpack.common.config.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,35 @@
const path = require('path');
const RemoveEmptyScriptsPlugin = require('webpack-remove-empty-scripts');

const ParagonWebpackPlugin = require('../lib/plugins/paragon-webpack-plugin/ParagonWebpackPlugin');
const {
getParagonThemeCss,
getParagonCacheGroups,
getParagonEntryPoints,
} = require('./data/paragonUtils');

const paragonThemeCss = getParagonThemeCss(process.cwd());
const brandThemeCss = getParagonThemeCss(process.cwd(), { isBrandOverride: true });

module.exports = {
entry: {
app: path.resolve(process.cwd(), './src/index'),
/**
* The entry points for the Paragon theme CSS. Example: ```
* {
* "paragon.theme.core": "/path/to/node_modules/@openedx/paragon/dist/core.min.css",
* "paragon.theme.variants.light": "/path/to/node_modules/@openedx/paragon/dist/light.min.css"
* }
*/
...getParagonEntryPoints(paragonThemeCss),
/**
* The entry points for the brand theme CSS. Example: ```
* {
* "paragon.theme.core": "/path/to/node_modules/@openedx/brand/dist/core.min.css",
Copy link
Member

Choose a reason for hiding this comment

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

nit: Similar to other comments; currently consuming MFEs install the brand package as @edx/brand.

* "paragon.theme.variants.light": "/path/to/node_modules/@openedx/brand/dist/light.min.css"
Copy link
Member

Choose a reason for hiding this comment

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

nit: Similar to other comments; currently consuming MFEs install the brand package as @edx/brand.

* }
*/
...getParagonEntryPoints(brandThemeCss),
},
output: {
path: path.resolve(process.cwd(), './dist'),
Expand All @@ -19,6 +46,23 @@ module.exports = {
},
extensions: ['.js', '.jsx', '.ts', '.tsx'],
},
optimization: {
splitChunks: {
chunks: 'all',
cacheGroups: {
...getParagonCacheGroups(paragonThemeCss),
...getParagonCacheGroups(brandThemeCss),
},
},
},
plugins: [
// RemoveEmptyScriptsPlugin get rid of empty scripts generated by webpack when using mini-css-extract-plugin
// This helps to clean up the final bundle application
// See: https://www.npmjs.com/package/webpack-remove-empty-scripts#usage-with-mini-css-extract-plugin

new RemoveEmptyScriptsPlugin(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be nice to add a comment somewhere explaining why this plugin is being used. The readme on npm says

Warning

This plugin is the Emergency Crutch 🩼 for the mini-css-extract-plugin issue.
The mini-css-extract-plugin extract CSS, but not eliminate a generated empty JS file.

I assume that is why this is being used, but it'd be nice to confirm/document that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @brian-smith-tcril, thanks for your review, I addressed this comment and rebased the branch.

new ParagonWebpackPlugin(),
],
ignoreWarnings: [
// Ignore warnings raised by source-map-loader.
// some third party packages may ship miss-configured sourcemaps, that interrupts the build
Expand Down
1 change: 1 addition & 0 deletions config/webpack.dev-stage.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ module.exports = merge(commonConfig, {
new HtmlWebpackPlugin({
inject: true, // Appends script tags linking to the webpack bundles at the end of the body
template: path.resolve(process.cwd(), 'public/index.html'),
chunks: ['app'],
FAVICON_URL: process.env.FAVICON_URL || null,
OPTIMIZELY_PROJECT_ID: process.env.OPTIMIZELY_PROJECT_ID || null,
NODE_ENV: process.env.NODE_ENV || null,
Expand Down
Loading