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

KirylHatalski · 2024-10-14T14:57:01Z

oatymart

Code changes look alright.

I'm pretty sure the reason tests are failing is due to async version upgrade. I checked out develop versions of package.json and package-lock, installed them (tests passed), then installed async@3 (tests failed).

I guess it is related to the following dependency chain, but I can't explain the precise error:

npm ls async
@oat-sa/tao-core-sdk@3.2.1
├── async@3.2.6
└─┬ handlebars@1.3.0
  └─┬ uglify-js@2.3.6
    └── async@0.2.10

oatymart · 2024-10-14T21:56:39Z

package.json

@@ -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",


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...

oatymart · 2024-10-14T21:56:42Z

package.json

        "eslint": "^8.39.0",
        "fetch-mock": "^9.11.0",
        "glob": "^8.1.0",
-        "handlebars": "1.3.0",
+        "handlebars": "4.7.7",


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).

src/core/polling.js

src/core/store/indexeddb.js

jsconan

Added a few comments. Mainly my concern is regarding the replacement of URI with relative paths.

I'm not sure it will work fine with TAO Terre, which still uses AMD. Moreover, if you want a proper handling of ESM, every import should have the file extension. Here again, I'm not sure about the impact on the AMD consumers.

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.

I see you did convert some files to ES6, which is nice, but then why is it half way?

jsconan · 2024-10-15T06:23:55Z

src/core/asyncProcess.js

+import Promise from './promise';
+import eventifier from './eventifier';


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.

jsconan · 2024-10-15T06:29:31Z

src/core/asyncProcess.js

@@ -52,7 +52,7 @@ function asyncProcessFactory() {
         * @returns {boolean} - Returns true if the process can be started
         */
        start: function start(cb) {


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) {

jsconan · 2024-10-15T06:30:15Z

src/core/filter/filters.js

-var Filters = {
-    register: function(name, filter) {
+const Filters = {
+    register: function (name, filter) {


Suggested change

register: function (name, filter) {

register(name, filter) {

jsconan · 2024-10-15T06:34:15Z

src/core/mouseEvent.js

@@ -53,8 +52,8 @@ var allowedEvents = [
 * @param {*} eventOptions
 * @returns {Event}
 */
-var createEvent = function createEvent(eventName, eventOptions) {
-    var event;
+const createEvent = function createEvent(eventName, eventOptions) {


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

Usually, when you see var something = function() { ... }, this is either a developer's taste (which is IMO debatable), or because the function needs to be redefined later on. If the function is immutable, then the best practice is to use a named function instead of const something = function() { ... }. Why? because function hoists, while const is lexically scoped. This also removes redundant and cumbersome naming.

suggestion:

Suggested change

const createEvent = function createEvent(eventName, eventOptions) {

function createEvent(eventName, eventOptions) {

jsconan · 2024-10-15T06:54:43Z

build/rollup.config.js

@@ -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";


Suggested change

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

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

jsconan · 2024-10-15T06:55:31Z

src/core/asyncProcess.js

+import Promise from './promise';
+import eventifier from './eventifier';


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';

jsconan · 2024-10-15T06:56:22Z

src/core/asyncProcess.js

 */
 /**
 * @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';


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

… the pathes adding

github-actions · 2024-10-23T09:58:30Z

Coverage Report

Totals
Statements:	90.55% ( 2625 / 2899 )
Methods:	94.87% ( 573 / 604 )

Karol-Stelmaczonek · 2024-10-23T10:11:46Z

build/path.js

@@ -19,12 +19,10 @@
 /**
 * This file contains path definitions for build scripts.
 */
-const path = require('path');
+import path from "path";


Use single quotes

Karol-Stelmaczonek · 2024-10-23T10:11:52Z

build/rollup.config.js

@@ -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.js";


Use single quotes

Karol-Stelmaczonek · 2024-10-23T10:12:19Z

src/core/collections.js

@@ -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


Karol-Stelmaczonek · 2024-10-23T10:12:44Z

src/core/customEvent.js

@@ -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) 2015-2019 (original work) Open Assessment Technologies SA ;
+ * Copyright (c) 2015-2024 (original work) Open Assessment Technologies SA ;


Karol-Stelmaczonek · 2024-10-23T10:13:00Z

src/core/dataattrhandler.js

@@ -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) 2013-2019 (original work) Open Assessment Technologies SA ;
+ * Copyright (c) 2013-2024 (original work) Open Assessment Technologies SA ;


Karol-Stelmaczonek · 2024-10-23T10:20:31Z

src/util/locale.js

@@ -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-2021 (original work) Open Assessment Technologies SA;
+ * Copyright (c) 2016-2024 (original work) Open Assessment Technologies SA;


rollback file

Karol-Stelmaczonek · 2024-10-23T10:20:42Z

src/util/regexEscape.js

@@ -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) 2015-2019 (original work) Open Assessment Technologies SA ;
+ * Copyright (c) 2015-2024 (original work) Open Assessment Technologies SA ;


rollback file

Karol-Stelmaczonek · 2024-10-23T10:20:53Z

src/util/strLimiter.js

@@ -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) 2018-2022 Open Assessment Technologies SA
+ * Copyright (c) 2018-2024 Open Assessment Technologies SA


rollback file

Karol-Stelmaczonek · 2024-10-23T10:21:05Z

src/util/url.js

@@ -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) 2015-2019 (original work) Open Assessment Technologies SA;
+ * Copyright (c) 2015-2024 (original work) Open Assessment Technologies SA;


rollback file

Karol-Stelmaczonek · 2024-10-23T10:21:16Z

src/util/urlParser.js

@@ -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) 2013-2019 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);
+ * Copyright (c) 2013-2024 (original work) Open Assessment Technologies SA (under the project TAO-PRODUCT);


rollback file

oatymart

I see some of the tests, after refactoring, do not seem to test the useful things any more.
You've done this to avoid failures based on ESM dependency module state?
I wonder if we should remove more, or take a deeper look into how to fix the testing set-up to pass.
@jsconan do you have any thoughts on this?

oatymart · 2024-10-23T12:18:41Z

test/core/dataProvider/proxy/ajax/test.js

-                assert.ok(false, 'The promise should not be rejected');
-                // eslint-disable-next-line
-                console.error(err);
+            .catch(function () {


Does it mean the result Promise previously resolved but now rejects?
If that's the case, it's testing the opposite of what's expected.
Is this test worth having any more?

Mocking with current way is not possible on ESM implementation, so now call always falling. I left this catch because this promise chain is required to keep previous asserts working.
I can move ready state to previous then block as consensuses.

oatymart · 2024-10-23T12:18:47Z

test/core/dataProvider/proxy/ajax/test.js

-                assert.ok(false, 'The promise should not be rejected');
-                // eslint-disable-next-line
-                console.error(err);
+            .catch(function () {


Same comment as above, should it really go into a catch to complete the test?

yairkukielka · 2024-10-24T16:21:02Z

When I run the tests I get a list of tests passing and then

...
/Users/yair/p/tao-core-sdk-fe/src/util/typeCaster.js → dist/util...
created dist/util in 7ms

/Users/yair/p/tao-core-sdk-fe/src/util/url.js → dist/util...
created dist/util in 37ms

/Users/yair/p/tao-core-sdk-fe/src/util/urlParser.js → dist/util...
created dist/util in 33ms

/Users/yair/p/tao-core-sdk-fe/src/util/wrapLongWords.js → dist/util...
created dist/util in 16ms

Server is listening on http://127.0.0.1:57602/test
..............................................................................
Timeout : test/core/request/test.html # Test run could not finish in 30000ms

Am I the only one?

oatymart · 2024-10-28T08:08:43Z

@KirylHatalski we also need to know if there has been a successful install of the new package in TAO 3.x.
If we're unable to use it there, it will be a blocker. You should consider deploying a Construct environment for a regression test.

Karol-Stelmaczonek

New code is covered by tests (if applicable)
Tests are running successfully (old and new ones) on my local machine (if applicable)
New code is respecting code style rules
New code is respecting best practices
New code is not subject to concurrency issues (if applicable)
Feature is working correctly on my local machine (if applicable)
Acceptance criteria are respected
Pull request title and description are meaningful

KirylHatalski · 2024-10-29T09:32:56Z

Package published as 4.0.0-alpha.1 and ready for checks.

yairkukielka

Version 4.0.0-alpha.1 was released. I tried it in control center and it didn't work.

My idea was that we could remove this moduleLoader mock that we use in all our apps and also remove this line, to help the bundler (rolllup/vite) resolve the import from 'core/.../;` lines.

[{ find: 'core/moduleLoader', replacement: path.resolve(srcDir, 'core', 'moduleLoader') }](https://github.com/oat-sa/tao-control-center/blob/develop/frontend/vite.config.js#L52)

If I remove the mock from my project and install version 4.0.0-alpha.1 I get this error in the console. The logger module isn't found.

In the sdk code I see

return Promise.all(
                             amdModules.map(module => import(/* webpackIgnore: true */ `${module}`))
                         ).then(loadedModules => loadedModules.map(module => module.default));

I don't think that will work in ESM, since import('${module}') is trying to import a variable value, but that wouldn't work in a dynamic import. The import needs a string literal. That's why we have this in our moduleLoader mock

return Promise.all(
                        amdModules.map(mod => {
                            switch (mod) {
                                case 'core/logger/console':
                                    return import('core/logger/console');
                            }
                        })
                    ).then(loadedModules => loadedModules.map(res => res && res.default));

So import('core/logger/console') works if we have

              {
                    find: 'core',
                    replacement: path.resolve(nodeModulesRoot, '@oat-sa', 'tao-core-sdk', 'src', 'core')
                },

in our app config, to help the bundler find the 'core/logger/console' module. But if we want to remove all the extra config what we need for it to work as a ESM package is

return Promise.all(
                        amdModules.map(mod => {
                            switch (mod) {
                                case 'core/logger/console':
                                    return import('./logger/console');
                            }
                        })
                    ).then(loadedModules => loadedModules.map(res => res && res.default));

which is the relative path to the ./logger/console.js file. So maybe a solution to this, compatible with AMD and ESM would be:


            const loadModules = (amdModules = []) => {
                if (_.isArray(amdModules) && amdModules.length) {
                    if (typeof window.define === 'function' && window.define.amd) {
                        return new Promise((resolve, reject) => {
                            window.require(
                                amdModules,
                                (...loadedModules) => resolve(loadedModules),
                                err => {
                                    reject(err);
                                }
                            );
                        });
                    } else {
                        return Promise.all(
                            amdModules.map(mod => {
                                switch (mod) {
                                    case 'core/logger/console':
                                        return import('./logger/console');
                                }
                            })
                        ).then(loadedModules => loadedModules.map(res => res && res.default));
                    }
                }
                return Promise.resolve();
            };

github-actions · 2024-10-30T13:35:36Z

Version

❕ Some commits are not using the conventional commits formats. They will be ignored in version management.

Target Version	3.2.2
Last version	3.2.1

There are 0 BREAKING CHANGE, 0 feature, 0 fix

jsconan

The code looks good.
Pay attention to some "typos" that you fixed that were not typos and that now are... 😄

Regarding the overall change, it does not work fine with TAO where multiple modules cannot load. Maybe is there something missing to update on TAO, but the PR description is pretty terse regarding the change...

jsconan · 2024-10-30T14:23:46Z

src/core/request.js

@@ -53,7 +53,7 @@ const logger = loggerFactory('core/request');
 * @param {Object} response - the server body response as plain object
 * @param {String} fallbackMessage - the error message in case the response isn't correct
 * @param {Number} httpCode - the response HTTP code
- * @param {Boolean} httpSent - the sent status
+ * @param {Boolean} httpSent - the scent status


chore: Funny, but not accurate! Computers cannot smell... 🤣

jsconan · 2024-10-30T14:26:58Z

src/core/requireIfExists.js

@@ -28,7 +27,7 @@ import Promise from 'core/promise';
 function requireIfExists(uri) {
    // the promise will always be resolved
    return new Promise(function(resolve) {
-        // if a require issue occurs, fallback to an empty resource
+        // if a requirement issue occurs, fallback to an empty resource


chore: I think the initial term was good: when the require API fails...

outdated

Kirill-Hatalski added 2 commits October 10, 2024 19:34

chore: [wip] initial refactoring and package json updates

3a178dd

chore: [wip] unit test fixes

9d91ea2

oatymart reviewed Oct 14, 2024

View reviewed changes

jsconan reviewed Oct 15, 2024

View reviewed changes

Kirill-Hatalski added 3 commits October 15, 2024 12:42

chore: [wip] async and handlebars version rollback, and extension for…

d536bd9

… the pathes adding

chore: [wip] additional es6 code refactoring

463c3ce

chore: Remove or fix broken tests

ea07a2c

KirylHatalski marked this pull request as ready for review October 21, 2024 14:25

KirylHatalski changed the title ~~[WIP] moving from AMD to ESM~~ [TR-6197] Moving from AMD to ESM Oct 21, 2024

KirylHatalski requested review from jsconan, oatymart, yairkukielka, Karol-Stelmaczonek and viktar-dzmitryieu-tao October 21, 2024 14:27

Kirill-Hatalski added 2 commits October 23, 2024 12:16

chore: async removing and few clearcode

9063c51

chore: removing promise polyfill from tests

dcc0b32

Karol-Stelmaczonek reviewed Oct 23, 2024

View reviewed changes

oatymart reviewed Oct 23, 2024

View reviewed changes

chore: rollbacks files without codechanges and codereview changes

05b7f16

Kirill-Hatalski added 2 commits October 28, 2024 11:29

chore: fix for tests catches

14dbd03

chore: code review fix

713d634

Karol-Stelmaczonek previously approved these changes Oct 28, 2024

View reviewed changes

chore: rollback files with unnecessary fixes

8a251ab

yairkukielka reviewed Oct 29, 2024

View reviewed changes

Kirill-Hatalski added 6 commits October 29, 2024 17:52

chore: relative path exception for logger fix

26afa4f

chore:[WIP] dynamic import try, unit tests will DOWN

ca0d676

chore: export fix

bde96c0

chore: export fix, correct

3ae5088

chore: separating build for esm and amd

2203e95

chore: version bump

4d939dd

jsconan reviewed Oct 30, 2024

View reviewed changes

Karol-Stelmaczonek self-requested a review November 4, 2024 09:48

		import Promise from './promise';
		import eventifier from './eventifier';

	const createEvent = function createEvent(eventName, eventOptions) {
	function createEvent(eventName, eventOptions) {

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

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

Are you sure you want to change the base?

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

Conversation

KirylHatalski commented Oct 14, 2024 • edited by jira bot Loading

oatymart left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsconan left a comment • edited Loading

Choose a reason for hiding this comment

jsconan Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 23, 2024 • edited Loading

Coverage Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oatymart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yairkukielka commented Oct 24, 2024 • edited Loading

oatymart commented Oct 28, 2024

Karol-Stelmaczonek left a comment

Choose a reason for hiding this comment

KirylHatalski commented Oct 29, 2024

yairkukielka left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2024

Version

jsconan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KirylHatalski commented Oct 14, 2024 •

edited by jira bot

Loading

oatymart left a comment •

edited

Loading

jsconan left a comment •

edited

Loading

jsconan Oct 15, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

yairkukielka commented Oct 24, 2024 •

edited

Loading

yairkukielka left a comment •

edited

Loading