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.

Sign up for GitHub

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

Jump to bottom

Make NPM post-processing agnostic to node_modules location #31519

Closed
wants to merge 3 commits into from
Closed

Make NPM post-processing agnostic to node_modules location #31519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a2ee88
refactor: build: reimplement `process_npm_assets` as shell script
kdmccormick Jan 6, 2023
1c65ca1
build: make node_modules path configurable in copy-node-modules.sh
kdmccormick Jan 17, 2023
1cc3fe3
build: copy edx-ui-toolkit instead of symlinking to it
kdmccormick Jan 9, 2023
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 common/static/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
xmodule
edx-ui-toolkit
1 change: 0 additions & 1 deletion common/static/edx-ui-toolkit/js
View file
Open in desktop

This file was deleted.

84 changes: 1 addition & 83 deletions pavelib/assets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,39 +46,6 @@
path('node_modules'),
]

# A list of NPM installed libraries that should be copied into the common
# static directory.
# If string ends with '/' then all file in the directory will be copied.
NPM_INSTALLED_LIBRARIES = [
'backbone.paginator/lib/backbone.paginator.js',
'backbone/backbone.js',
'bootstrap/dist/js/bootstrap.bundle.js',
'hls.js/dist/hls.js',
'jquery-migrate/dist/jquery-migrate.js',
'jquery.scrollto/jquery.scrollTo.js',
'jquery/dist/jquery.js',
'moment-timezone/builds/moment-timezone-with-data.js',
'moment/min/moment-with-locales.js',
'picturefill/dist/picturefill.js',
'requirejs/require.js',
'underscore.string/dist/underscore.string.js',
'underscore/underscore.js',
'@edx/studio-frontend/dist/',
'which-country/index.js'
]

# A list of NPM installed developer libraries that should be copied into the common
# static directory only in development mode.
NPM_INSTALLED_DEVELOPER_LIBRARIES = [
'sinon/pkg/sinon.js',
'squirejs/src/Squire.js',
]

# Directory to install static vendor files
NPM_JS_VENDOR_DIRECTORY = path('common/static/common/js/vendor')
NPM_CSS_VENDOR_DIRECTORY = path("common/static/common/css/vendor")
NPM_CSS_DIRECTORY = path("common/static/common/css")

# system specific lookup path additions, add sass dirs if one system depends on the sass files for other systems
SASS_LOOKUP_DEPENDENCIES = {
'cms': [path('lms') / 'static' / 'sass' / 'partials', ],
Expand Down Expand Up @@ -604,60 +571,11 @@ def process_npm_assets():
"""
Process vendor libraries installed via NPM.
"""
def copy_vendor_library(library, skip_if_missing=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this process_npm_assets function now call the process-npm-assets.sh script? It seems to me that we would need this to be backward compatible.
As a side note, the openedx-assets.py script in Tutor calls the process_npm_assets function, so we would have to modify it if the function does not call the bash script: https://github.com/overhangio/tutor/blob/b903c69fac95e797532288d4e6f60530faf1db94/tutor/templates/build/openedx/bin/openedx-assets#L117

Copy link
Member Author

Choose a reason for hiding this comment

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

Should this process_npm_assets function now call the process-npm-assets.sh script? It seems to me that we would need this to be backward compatible.

It does, see this line of the diff. And yes, goal is backwards-compatibility.

As a side note, the openedx-assets.py script in Tutor calls the process_npm_assets function, so we would have to modify it if the function does not call the bash script: https://github.com/overhangio/tutor/blob/b903c69fac95e797532288d4e6f60530faf1db94/tutor/templates/build/openedx/bin/openedx-assets#L117

For what it's worth, I do plan to change Tutor's openedx-assets.py script to call this bash script directly--we will need to do so in order to pass in the --node-modules=/openedx/node_modules. That change is not necessary in order to merge this current PR, though.

"""
Copies a vendor library to the shared vendor directory.
"""
if library.startswith('node_modules/'):
library_path = library
else:
library_path = f'node_modules/{library}'

if library.endswith('.css') or library.endswith('.css.map'):
vendor_dir = NPM_CSS_VENDOR_DIRECTORY
else:
vendor_dir = NPM_JS_VENDOR_DIRECTORY
if os.path.exists(library_path):
sh('/bin/cp -rf {library_path} {vendor_dir}'.format(
library_path=library_path,
vendor_dir=vendor_dir,
))
elif not skip_if_missing:
raise Exception(f'Missing vendor file {library_path}')

def copy_vendor_library_dir(library_dir, skip_if_missing=False):
"""
Copies all vendor libraries in directory to the shared vendor directory.
"""
library_dir_path = f'node_modules/{library_dir}'
print(f'Copying vendor library dir: {library_dir_path}')
if os.path.exists(library_dir_path):
for dirpath, _, filenames in os.walk(library_dir_path):
for filename in filenames:
copy_vendor_library(os.path.join(dirpath, filename), skip_if_missing=skip_if_missing)

# Skip processing of the libraries if this is just a dry run
if tasks.environment.dry_run:
tasks.environment.info("install npm_assets")
return

# Ensure that the vendor directory exists
NPM_JS_VENDOR_DIRECTORY.mkdir_p()
NPM_CSS_DIRECTORY.mkdir_p()
NPM_CSS_VENDOR_DIRECTORY.mkdir_p()

# Copy each file to the vendor directory, overwriting any existing file.
print("Copying vendor files into static directory")
for library in NPM_INSTALLED_LIBRARIES:
if library.endswith('/'):
copy_vendor_library_dir(library)
else:
copy_vendor_library(library)

# Copy over each developer library too if they have been installed
print("Copying developer vendor files into static directory")
for library in NPM_INSTALLED_DEVELOPER_LIBRARIES:
copy_vendor_library(library, skip_if_missing=True)
sh(cmd('/bin/sh', 'scripts/assets/copy-node-modules.sh'))


@task
Expand Down
145 changes: 145 additions & 0 deletions scripts/assets/copy-node-modules.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
#!/bin/sh
#
# Copy certain node_modules to other places in edx-platform.
#
# Run this from the root of edx-platform, after node_modules are installed,
# but before trying to compile static assets.
#
# Background:
#
# Earlier in edx-platform's development, JS and CSS dependencies were
# committed directly (a.k.a. "vendored in") to the platform.
# Later on, as NPM became popular, new edx-platform frontends began
# using package.json to specify dependencies, and of course those new
# dependencies were installed into node_modules.
#
# Unfortunately, not all old pages were updated to use package.json.
# However, rather then continuing to vendor-in the dependencies for
# the old pages, it was decided to copy the required files from
# node_modules to the old "vendor" directories. That way, the old
# pages' dependencies would remain synced with the newer pages'
# dependencies. That is what this script does.
#
# This was formerly implemented in pavelib/assets.py. As we are moving
# away from paver, it was reimplemented in this shell script.
#
# NOTE: This uses plain POSIX `sh` instead of `bash` for the purpose of
# maximum portability.

USAGE="Usage: $0 [ (-n|--node-modules) NODE_MODULES_PATH ]"

# By default, we look for node_modules in the current directory.
# Some Open edX distributions may want node_modules to be located somewhere
# else, so we let this be configured with -n|--node-modules.
NODE_MODULES_PATH="./node_modules"

# Vendor destination paths for assets.
# These are not configurable yet, but that could be changed if necessary.
JS_VENDOR_PATH="./common/static/common/js/vendor"
CSS_VENDOR_PATH="./common/static/common/css/vendor"
EDX_UI_TOOLKIT_VENDOR_PATH="./common/static/edx-ui-toolkit"

# Enable stricter sh behavior.
set -eu

# Parse options.
while [ $# -gt 0 ]; do
case $1 in
-n|--node-modules)
shift
if [ $# -eq 0 ]; then
echo "Error: Missing value for -n/--node-modules"
echo "$USAGE"
exit 1
fi
NODE_MODULES_PATH="$1"
shift
;;
--help)
echo "$USAGE"
exit 0
;;
*)
echo "Error: Unrecognized option: $1"
echo "$USAGE"
exit 1
;;
esac
done

echo "-------------------------------------------------------------------------"
echo "Copying shared node_modules to 'vendor' directories..."
echo " Working directory == '$(pwd)'"
echo " NODE_MODULES_PATH == '$NODE_MODULES_PATH'"
echo " CSS_VENDOR_PATH == '$CSS_VENDOR_PATH'"
echo " JS_VENDOR_PATH == '$JS_VENDOR_PATH'"
echo " EDX_UI_TOOLKIT_VENDOR_PATH == '$EDX_UI_TOOLKIT_VENDOR_PATH'"
echo "-------------------------------------------------------------------------"

# Input validation
if ! [ -d "$NODE_MODULES_PATH" ]; then
echo "Error: not a directory: $NODE_MODULES_PATH"
exit 1
fi
if ! [ -d ./common ]; then
echo "Error: not a directory: ./common"
echo "Hint: $0 must be run from the root of the edx-platform directory!"
exit 1
fi

# Echo lines back to user.
set -x

# Create vendor directories.
mkdir -p "$JS_VENDOR_PATH"
mkdir -p "$CSS_VENDOR_PATH"
mkdir -p "$EDX_UI_TOOLKIT_VENDOR_PATH"

# Copy studio-frontend assets into into vendor directory.
find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \
-type f \! -name \*.css \! -name \*.css.map -print0 | \
xargs --null cp --target-directory="$JS_VENDOR_PATH"
find "$NODE_MODULES_PATH/@edx/studio-frontend/dist" \
-type f \( -name \*.css -o -name \*.css.map \) -print0 | \
xargs --null cp --target-directory="$CSS_VENDOR_PATH"

# Copy edx-ui-toolkit's JS to into its own special directory.
# Note: $EDX_UI_TOOLKIT_VENDOR_PATH/js used to be a git-managed symlink.
# To avoid confusing behavior for folks who still have the js/ symlink
# hanging around in their repo, the safest thing to do is to remove
# the target js/ before copying in js/ fresh from the source.
rm -rf "$EDX_UI_TOOLKIT_VENDOR_PATH/js"
cp --recursive \
"$NODE_MODULES_PATH/edx-ui-toolkit/src/js" \
"$EDX_UI_TOOLKIT_VENDOR_PATH"

# Copy certain node_modules scripts into "vendor" directory.
cp --force \
"$NODE_MODULES_PATH/backbone.paginator/lib/backbone.paginator.js" \
"$NODE_MODULES_PATH/backbone/backbone.js" \
"$NODE_MODULES_PATH/bootstrap/dist/js/bootstrap.bundle.js" \
"$NODE_MODULES_PATH/hls.js/dist/hls.js" \
"$NODE_MODULES_PATH/jquery-migrate/dist/jquery-migrate.js" \
"$NODE_MODULES_PATH/jquery.scrollto/jquery.scrollTo.js" \
"$NODE_MODULES_PATH/jquery/dist/jquery.js" \
"$NODE_MODULES_PATH/moment-timezone/builds/moment-timezone-with-data.js" \
"$NODE_MODULES_PATH/moment/min/moment-with-locales.js" \
"$NODE_MODULES_PATH/picturefill/dist/picturefill.js" \
"$NODE_MODULES_PATH/requirejs/require.js" \
"$NODE_MODULES_PATH/underscore.string/dist/underscore.string.js" \
"$NODE_MODULES_PATH/underscore/underscore.js" \
"$NODE_MODULES_PATH/which-country/index.js" \
"$JS_VENDOR_PATH"

# Copy certain node_modules developer scripts into "vendor" directory.
# Since they're just developer libraries, they might not exist in a production build pipeline.
# So, let the error pass silently (`... || true`) if the copy fails.
cp --force "$NODE_MODULES_PATH/sinon/pkg/sinon.js" "$JS_VENDOR_PATH" || true
cp --force "$NODE_MODULES_PATH/squirejs/src/Squire.js" "$JS_VENDOR_PATH" || true

# Stop echoing.
set +x

echo "-------------------------------------------------------------------------"
echo "Done copying shared node_modules."
echo "-------------------------------------------------------------------------"