Skip to content

Use handlebars from node_modules (on CI) #17340

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

Merged
skjnldsv merged 1 commit into from
Oct 1, 2019
Merged

Use handlebars from node_modules (on CI) #17340

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion .drone.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ steps:
- name: handlebars
image: node
commands:
- npm install handlebars -g
- npm i
Copy link
Contributor

@kesselb kesselb Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

npm i will install the latest handlebars version in the range ^4.2.1.
npm ci would install the exact version from package-lock.json.
Fake news 🙈

Just as information. I was running into troubles with npm i on ci in the past.

Copy link
Member Author

@ChristophWurst ChristophWurst Sep 30, 2019

Choose a reason for hiding this comment

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

Where's that documented? Can't find the info at https://docs.npmjs.com/cli/ci.

Copy link
Member Author

@ChristophWurst ChristophWurst Sep 30, 2019

Choose a reason for hiding this comment

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

the ^ version range is a problem in general. If you add a new dependency with npm i -S xxx to Nextcloud npm will update the package-lock.json with a newer version of handlebars. Not quite what we want.

This also caused issues in the past (@juliushaertl 👀)

Copy link
Member

@skjnldsv skjnldsv Sep 30, 2019

Choose a reason for hiding this comment

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

@kesselb doesn't npm ci only make sure to clean the node_modules folder?

Copy link
Member

@skjnldsv skjnldsv Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This command is similar to npm-install, except it’s meant to be used in automated environments such as test platforms, continuous integration, and deployment – or any situation where you want to make sure you’re doing a clean install of your dependencies. It can be significantly faster than a regular npm install by skipping certain user-oriented features. It is also more strict than a regular install, which can help catch errors or inconsistencies caused by the incrementally-installed local environments of most npm users.

In short, the main differences between using npm install and npm ci are:

  • The project must have an existing package-lock.json or npm-shrinkwrap.json.
  • If dependencies in the package lock do not match those in package.json, npm ci will exit with an error, instead of updating the package lock.
  • npm ci can only install entire projects at a time: individual dependencies cannot be added with this command.
  • If a node_modules is already present, it will be automatically removed before npm ci begins its install.
  • It will never write to package.json or any of the package-locks: installs are essentially frozen.

Oh!

Copy link
Member Author

@ChristophWurst ChristophWurst Sep 30, 2019

Choose a reason for hiding this comment

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

We should actually use this in our makefiles too see_no_evil

If you prefer slower installs, sure, go ahead 😉

Also, you won't see the security alerts as I just tested.

Copy link
Contributor

@kesselb kesselb Sep 30, 2019

Choose a reason for hiding this comment

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

Where's that documented? Can't find the info at https://docs.npmjs.com/cli/ci.

Fair question.

npm i will install the latest handlebars version in the range ^4.2.1

I'm not 100% sure. We had less issues with conflicting package-lock.json or failing builds after we switched to npm ci (also locally). I haven't compared my node_modules folder with others in depth 🙈 A positive side effect of npm ci is that package-lock.json is not modified.

Copy link
Member

@skjnldsv skjnldsv Sep 30, 2019

Choose a reason for hiding this comment

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

unless you specify a package, npm i seems to install the version written on package.json/lock :)

Copy link
Member Author

@ChristophWurst ChristophWurst Oct 1, 2019

Choose a reason for hiding this comment

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

npm i seems to install the version written on package.json/lock

Which also is the sole purpose of the lock file, right? :)

Copy link
Member

@skjnldsv skjnldsv Oct 1, 2019

Choose a reason for hiding this comment

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

Let's merge then! :)

- ./build/compile-handlebars-templates.sh

trigger:
Expand Down
12 changes: 6 additions & 6 deletions build/compile-handlebars-templates.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,22 @@ REPODIR=`git rev-parse --show-toplevel`
cd $REPODIR

# Settings
handlebars -n OC.Settings.Templates apps/settings/js/templates -f apps/settings/js/templates.js
node node_modules/handlebars/bin/handlebars -n OC.Settings.Templates apps/settings/js/templates -f apps/settings/js/templates.js

# Systemtags
handlebars -n OC.SystemTags.Templates core/js/systemtags/templates -f core/js/systemtags/templates.js
node node_modules/handlebars/bin/handlebars -n OC.SystemTags.Templates core/js/systemtags/templates -f core/js/systemtags/templates.js

# Share
handlebars -n OC.Share.Templates core/js/share -f core/js/sharetemplates.js
node node_modules/handlebars/bin/handlebars -n OC.Share.Templates core/js/share -f core/js/sharetemplates.js

# Files app
handlebars -n OCA.Files.Templates apps/files/js/templates -f apps/files/js/templates.js
node node_modules/handlebars/bin/handlebars -n OCA.Files.Templates apps/files/js/templates -f apps/files/js/templates.js

# Sharing
handlebars -n OCA.Sharing.Templates apps/files_sharing/js/templates -f apps/files_sharing/js/templates.js
node node_modules/handlebars/bin/handlebars -n OCA.Sharing.Templates apps/files_sharing/js/templates -f apps/files_sharing/js/templates.js

# Files external
handlebars -n OCA.Files_External.Templates apps/files_external/js/templates -f apps/files_external/js/templates.js
node node_modules/handlebars/bin/handlebars -n OCA.Files_External.Templates apps/files_external/js/templates -f apps/files_external/js/templates.js

if [[ $(git diff --name-only) ]]; then
echo "Please submit your compiled handlebars templates"
Expand Down