Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Get tests and builds working in current Node.js versions #195

Merged
merged 11 commits into from
Jan 25, 2024
Merged

Get tests and builds working in current Node.js versions #195

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8a4a744
Upgrade grunt-contrib-jasmine to v1.2
Mr0grog Jan 17, 2024
94acb4e
Upgrade grunt-jasmine-node to v0.3.1
Mr0grog Jan 17, 2024
7b77ecf
Update CI workflow to test in new Node.js versions
Mr0grog Jan 17, 2024
1e6bdba
Just drop grunt-jasmine-node
Mr0grog Jan 17, 2024
72182e5
Upgrade grunt-contrib-jasmine to v4, grunt-template-jasmine-istanbul …
Mr0grog Jan 17, 2024
a16ed9c
Turns out this does not in fact work on Node v6
Mr0grog Jan 18, 2024
53afb5e
Update Node tests to Jasmine 5.1
Mr0grog Jan 18, 2024
160fc4c
Fix issue with cookie support testing
Mr0grog Jan 18, 2024
bb00d41
Revert "Update Node tests to Jasmine 5.1"
Mr0grog Jan 18, 2024
f2bf952
Remove grunt-template-jasmine-istanbul
Mr0grog Jan 21, 2024
03cffed
Reword comments not to reference somethign we no longer use and that …
Mr0grog Jan 21, 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
3 changes: 3 additions & 0 deletions .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ indent_size = 2

[*.{yaml,yml}]
indent_size = 2

[vendor/grunt-template-jasmine-requirejs/**/*]
indent_size = 2
Comment on lines +26 to +27
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I probably should have done this when I vendored grunt-template-jasmine-requirejs. It was helpful when I had to make changes. 🤷

22 changes: 10 additions & 12 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v4
with:
node-version: 10
node-version: 20
cache: npm
cache-dependency-path: 'package.json'

- name: Install dependencies
run: npm install
run: npm install --legacy-peer-deps
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 --legacy-peer-deps option is required for grunt-template-jasmine-istanbul v0.6.0, since it depends on grunt-contrib-jasmine v2.x and we are now on v4.x. It’s less than ideal, but is only needed in order to install dev dependencies; a consumer of loglevel should not need it when running npm install loglevel in their package.

As noted in the PR description, I actually think the best solution if we want to remove this is to drop grunt-template-jasmine-istanbul altogether.

Copy link
Owner

Choose a reason for hiding this comment

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

This sounds like something that would make contribution difficult though, it would be nice if this worked out of the box so that contributors can just run npm install without problems.

You can run npm config --location=project set legacy-peer-deps=true to fix this - that'll create a .npmrc that will enable this setting by default for any npm command run in this repo, and then you can drop that option from here.

(But as noted in my other comment - I think we should just drop this instead, so you don't actually need to do that unless there's some other legacy peer deps issue elsewhere too)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I had not thought about adding a .npmrc file for this, good catch! But also, yeah, agree on the downsides anyway.


- name: JSHint
run: npm run lint
Expand All @@ -37,12 +37,12 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v4
with:
node-version: 10
node-version: 20
cache: npm
cache-dependency-path: 'package.json'

- name: Install dependencies
run: npm install
run: npm install --legacy-peer-deps

- name: Build
run: npm run dist-build
Expand All @@ -51,9 +51,7 @@ jobs:
runs-on: ubuntu-latest
strategy:
matrix:
# Ideally we'd also test on [12, 14, 16, 18, 20], but the current
# tooling does not support them.
node_version: [6, 8, 10]
node_version: [6, 8, 10, 12, 14, 16, 18, 20]

steps:
- uses: actions/checkout@v4
Expand All @@ -66,7 +64,7 @@ jobs:
cache-dependency-path: 'package.json'

- name: Install dependencies
run: npm install
run: npm install --legacy-peer-deps

- name: Unit Tests
run: npm run test-node
Expand All @@ -80,12 +78,12 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v4
with:
node-version: 8
node-version: 20
cache: npm
cache-dependency-path: 'package.json'

- name: Install dependencies
run: npm install
run: npm install --legacy-peer-deps

- name: Unit Tests
run: npm run test-browser
Expand All @@ -99,12 +97,12 @@ jobs:
- name: Install Node.js
uses: actions/setup-node@v4
with:
node-version: 10
node-version: 20
cache: npm
cache-dependency-path: 'package.json'

- name: Install dependencies
run: npm install
run: npm install --legacy-peer-deps

- name: Typescript Tests
run: npm run test-types
31 changes: 21 additions & 10 deletions Gruntfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

var Jasmine = require('jasmine');

module.exports = function (grunt) {
var jasmineRequireJsOptions = {
specs: 'test/*-test.js',
Expand Down Expand Up @@ -85,14 +87,9 @@ module.exports = function (grunt) {
}
}
},
"jasmine_node": {
test: {
options: {
match: "node-integration.",
matchall: true,
projectRoot: "./test",
useHelpers: false
}
jasmine_node: {
options: {
specs: ['test/node-integration.js']
}
},
open: {
Expand Down Expand Up @@ -155,7 +152,6 @@ module.exports = function (grunt) {
grunt.loadNpmTasks('grunt-contrib-concat');
grunt.loadNpmTasks('grunt-contrib-uglify');
grunt.loadNpmTasks('grunt-contrib-jasmine');
grunt.loadNpmTasks('grunt-jasmine-node');
grunt.loadNpmTasks('grunt-contrib-jshint');
grunt.loadNpmTasks('grunt-contrib-watch');

Expand All @@ -164,6 +160,22 @@ module.exports = function (grunt) {
grunt.loadNpmTasks('grunt-preprocess');
grunt.loadNpmTasks('grunt-contrib-clean');

// A simple version of `grunt-jasmine-node` that works in modern engines.
//
// NOTE: This is designed for Jasmine 2.4, which matches the version used
// in `grunt-contrib-jasmine`. If that package is updated, this should also
// be updated to match.
grunt.registerTask('jasmine_node', 'Run Jasmine in Node.js', function() {
var done = this.async();

var jasmine = new Jasmine({ projectBaseDir: __dirname });
jasmine.onComplete(function(success) {
done(success);
});

jasmine.execute(this.options().specs);
});

// Build a distributable release
grunt.registerTask('dist', ['test', 'dist-build']);
grunt.registerTask('dist-build', ['concat', 'uglify']);
Expand All @@ -178,5 +190,4 @@ module.exports = function (grunt) {

// Default task.
grunt.registerTask('default', 'test');

};
6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,14 @@
"grunt-contrib-clean": "^1.1.0",
"grunt-contrib-concat": "~0.5.0",
"grunt-contrib-connect": "^1.0.2",
"grunt-contrib-jasmine": "~1.0.3",
"grunt-contrib-jasmine": "^4.0.0",
"grunt-contrib-jshint": "^1.1.0",
"grunt-contrib-uglify": "^3.4.0",
"grunt-contrib-watch": "^1.1.0",
"grunt-jasmine-node": "~0.2.1",
"grunt-open": "~0.2.3",
"grunt-preprocess": "^5.1.0",
"grunt-template-jasmine-istanbul": "~0.4.0",
"grunt-template-jasmine-istanbul": "https://github.com/maenu/grunt-template-jasmine-istanbul.git#caedc636b9f9c11217a32d586717e3153edf9ba3",
"jasmine": "^2.4.1",
"typescript": "^3.5.1"
},
"keywords": [
Expand Down
11 changes: 8 additions & 3 deletions vendor/grunt-template-jasmine-requirejs/src/template-jasmine-requirejs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ function resolvePath(filepath) {
return path.resolve(filepath);
}

function moveRequireJs(grunt, task, versionOrPath) {
// LOGLEVEL-FORK: copying tempfiles now requires info from the `context` object.
function moveRequireJs(grunt, task, context, versionOrPath) {
var pathToRequireJS,
versionReg = /^(\d\.?)*$/;

Expand All @@ -74,8 +75,10 @@ function moveRequireJs(grunt, task, versionOrPath) {
throw new Error('local file path of requirejs [' + versionOrPath + '] was not found');
}
}
task.copyTempFile(pathToRequireJS,'require.js');
task.copyTempFile(pathToRequireJS, path.join(context.temp, 'require.js'));
}
// END LOGLEVEL-FORK


exports.process = function(grunt, task, context) {

Expand Down Expand Up @@ -149,7 +152,9 @@ exports.process = function(grunt, task, context) {
});
}

moveRequireJs(grunt, task, version);
// LOGLEVEL-FORK: this function now requires context info
moveRequireJs(grunt, task, context, version);
// END LOGLEVEL-FORK

context.serializeRequireConfig = function(requireConfig) {
var funcCounter = 0;
Expand Down
4 changes: 3 additions & 1 deletion vendor/grunt-template-jasmine-requirejs/src/templates/jasmine-requirejs.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
<script src="<%= temp ? temp + "/" : temp %>require.js"></script>

<% with (scripts) { %>
<% [].concat(jasmine, boot, helpers).forEach(function(script){ %>
<% /* LOGLEVEL-FORK: Include new script types used by grunt-contrib-jasmine v4 */ %>
<% [].concat(polyfills, jasmine, boot, boot2, helpers).forEach(function(script){ %>
<script src="<%= script %>"></script>
<% }) %>
<% /* END LOGLEVEL-FORK */ %>
<% }; %>

<script>
Expand Down
Loading