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

[TR-6197] Moving from AMD to ESM #190

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
+563 −980
Open

[TR-6197] Moving from AMD to ESM #190

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3a178dd
chore: [wip] initial refactoring and package json updates
Oct 10, 2024
9d91ea2
chore: [wip] unit test fixes
Oct 14, 2024
d536bd9
chore: [wip] async and handlebars version rollback, and extension for…
Oct 15, 2024
463c3ce
chore: [wip] additional es6 code refactoring
Oct 15, 2024
ea07a2c
chore: Remove or fix broken tests
Oct 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
12 changes: 5 additions & 7 deletions build/path.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@
/**
* This file contains path definitions for build scripts.
*/
const path = require('path');
import path from "path";
const rootPath = path.resolve(__dirname, '..');

module.exports = {
srcDir: path.resolve(rootPath, 'src'),
testDir: path.resolve(rootPath, 'test'),
outputDir: path.resolve(rootPath, 'dist'),
testOutputDir: path.resolve(rootPath, 'test')
};
export const srcDir = path.resolve(rootPath, 'src');
export const testDir = path.resolve(rootPath, 'test');
export const outputDir = path.resolve(rootPath, 'dist');
export const testOutputDir = path.resolve(rootPath, 'test');
2 changes: 1 addition & 1 deletion build/rollup.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import commonJS from 'rollup-plugin-commonjs';
import babel from 'rollup-plugin-babel';
import istanbul from 'rollup-plugin-istanbul';

const { srcDir, outputDir } = require('./path');
import { srcDir, outputDir } from "./path";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
import { srcDir, outputDir } from "./path";
import { srcDir, outputDir } from "./path.js";


const isDev = process.env.NODE_ENV === 'development';

Expand Down
93 changes: 31 additions & 62 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,15 @@
"displayName": "TAO Core SDK",
"description": "Core libraries of TAO",
"homepage": "https://github.com/oat-sa/tao-core-sdk-fe#readme",
"type": "module",
"files": [
"dist/",
"src/"
],
"exports": {
"./core/*": "./src/core/*.js",
"./util/*": "./src/util/*.js"
},
"license": "GPL-2.0",
"scripts": {
"test": "npx qunit-testrunner",
Expand Down Expand Up @@ -47,11 +52,11 @@
"@oat-sa/eslint-config-tao": "^2.0.0",
"@oat-sa/prettier-config": "^0.1.1",
"@oat-sa/tao-qunit-testrunner": "^2.0.0",
"async": "^0.2.10",
"async": "^3.2.6",
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe there was a specific reason we kept the async 0.x version for so long.
If the 3.x version is 100% equivalent, that's great, but we must be absolutely sure of it.
I'm not even sure where it is used in this library - need to check history books and see if another part of the TAO ecosystem breaks...

"eslint": "^8.39.0",
"fetch-mock": "^9.11.0",
"glob": "^8.1.0",
"handlebars": "1.3.0",
"handlebars": "4.7.7",
Copy link
Contributor

Choose a reason for hiding this comment

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

For handlebars I don't think we can upgrade this easily. It requires a big effort to synchronise across all TAO 3.x repos, see https://oat-sa.atlassian.net/wiki/spaces/FOUN/pages/144081031/Upgrade+Handlebars (a task which ended up blocked the last 2 times).

"jquery-mockjax": "^2.6.0",
"jquery-simulate": "^1.0.2",
"node-qunit-puppeteer": "^1.0.13",
Expand Down
16 changes: 8 additions & 8 deletions src/core/asyncProcess.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Copyright (c) 2016-2019 (original work) Open Assessment Technologies SA ;
* Copyright (c) 2016-2024 (original work) Open Assessment Technologies SA ;
*/
/**
* @author Jean-Sébastien Conan <jean-sebastien.conan@vesperiagroup.com>
*/
import _ from 'lodash';
import Promise from 'core/promise';
import eventifier from 'core/eventifier';
import Promise from './promise';
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Is this still needed? Promise was polyfilled, but it has been natively supported since ages.

import eventifier from './eventifier';
Copy link
Contributor

@jsconan jsconan Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

issue: I don't think the URI can be changed. Remember, the consumer still needs to access AMD-compatible URIs. Renaming the routes may create issues. I'm curious to know why such a change is necessary.

update: Ok, this is required by the ticket, actually. While it is worth checking with the Terre consumers, we should also add the file extension to each import as this is recommended by the standard and required by Vite.

Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: to properly follow the requirements, we also need to add the file extension

Suggested change
import Promise from './promise';
import eventifier from './eventifier';
import Promise from './promise.js';
import eventifier from './eventifier.js';


/**
* Defines a manager for async process with deferred steps.
Expand All @@ -34,8 +34,8 @@ import eventifier from 'core/eventifier';
* @trigger reject - When the process has finished on error
*/
function asyncProcessFactory() {
var running = false;
var steps = [];
let running = false;
let steps = [];

return eventifier({
/**
Expand All @@ -52,7 +52,7 @@ function asyncProcessFactory() {
* @returns {boolean} - Returns true if the process can be started
*/
start: function start(cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

quibble: Converting the code to use let and const instead of var is good, but why not continue and also use the shorthand?

Suggested change
start: function start(cb) {
start(cb) {

var started = false;
let started = false;
if (!running) {
steps = [];
running = true;
Expand Down Expand Up @@ -94,8 +94,8 @@ function asyncProcessFactory() {
* @returns {Promise} - Returns the finish promise
*/
done: function done(cb) {
var self = this;
var finish = Promise.all(steps);
const self = this;
const finish = Promise.all(steps);

finish
.then(function(data) {
Expand Down
4 changes: 2 additions & 2 deletions src/core/cachedStore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Copyright (c) 2016-2019 (original work) Open Assessment Technologies SA ;
* Copyright (c) 2016-2024 (original work) Open Assessment Technologies SA ;
*/
/**
* @author Jean-Sébastien Conan <jean-sebastien.conan@vesperiagroup.com>
*/
import store from 'core/store';
import store from './store';

/**
* The default name of the key storage indexing the persisted data
Expand Down
2 changes: 1 addition & 1 deletion src/core/collections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Copyright (c) 2016-2019 Open Assessment Technologies SA
* Copyright (c) 2016-2024 Open Assessment Technologies SA
*/

/**
Expand Down
13 changes: 6 additions & 7 deletions src/core/communicator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,15 @@
* along with this program; if not, write to the Free Software
* Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*
* Copyright (c) 2016-2019 (original work) Open Assessment Technologies SA ;
* Copyright (c) 2016-2024 (original work) Open Assessment Technologies SA ;
*/
/**
* @author Jean-Sébastien Conan <jean-sebastien.conan@vesperiagroup.com>
*/
import _ from 'lodash';
import Promise from 'core/promise';
import providerRegistry from 'core/providerRegistry';
import delegator from 'core/delegator';
import eventifier from 'core/eventifier';
import providerRegistry from './providerRegistry';
import delegator from './delegator';
import eventifier from './eventifier';

/**
* Some default config values
Expand Down Expand Up @@ -156,7 +155,7 @@ function communicatorFactory(providerName, config) {
},

/**
* Sends an messages through the communication implementation.
* Sends a messages through the communication implementation.
* @param {String} channel - The name of the communication channel to use
* @param {Object} message - The message to send
* @returns {Promise} The delegated provider's method must return a promise
Expand Down Expand Up @@ -227,7 +226,7 @@ function communicatorFactory(providerName, config) {
}
});

// all messages comes through a message event, then each is dispatched to the right channel
// all messages come through a message event, then each is dispatched to the right channel
communicator.on('message', function (channel, message) {
this.trigger(`channel-${channel}`, message);
});
Expand Down
9 changes: 4 additions & 5 deletions src/core/communicator/poll.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,8 @@
*/

import _ from 'lodash';
import pollingFactory from 'core/polling';
import Promise from 'core/promise';
import coreRequest from 'core/request';
import pollingFactory from '../polling';
import coreRequest from '../request';

/**
* Some default config values
Expand Down Expand Up @@ -68,7 +67,7 @@ const defaults = {
*
* Business logic errors can be implemented using the `error` *channel*.
* Network errors are handled by the AJAX implementation, and are forwarded to the `error` *event*.
* Additional network error handling can be achieve by the rejected send promises.
* Additional network error handling can be achieved by the rejected send promises.
*
* Malformed messages will be issued through the `malformed` channel
*
Expand Down Expand Up @@ -258,7 +257,7 @@ const pollProvider = {
});
this.messagesQueue.push(pending);

// force a send in the next throttle period
// force send in the next throttle period
this.throttledSend();

return promise;
Expand Down
Loading
Loading