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

oatymart · 2024-10-14T21:57:16Z

src/core/polling.js

@@ -115,8 +115,8 @@
 */

 import _ from 'lodash';
-import Promise from 'core/promise';
-import eventifier from 'core/eventifier';
+import Promise from './promise';


Can Promise be removed here too?

oatymart · 2024-10-14T21:57:22Z

src/core/store/indexeddb.js

@@ -22,9 +22,9 @@
 * @author Bertrand Chevrier <bertrand@taotesting.com>
 */
 import _ from 'lodash';
-import Promise from 'core/promise';
+import Promise from '../promise';


Can we remove it?

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-21T13:53:55Z

Version

Target Version	3.2.2
Last version	3.2.1

There are 0 BREAKING CHANGE, 0 feature, 0 fix

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

KirylHatalski commented Oct 14, 2024 •

edited by jira bot

Loading

oatymart left a comment •

edited

Loading

oatymart Oct 14, 2024

oatymart Oct 14, 2024

oatymart Oct 14, 2024

oatymart Oct 14, 2024

jsconan left a comment •

edited

Loading

jsconan Oct 15, 2024 •

edited

Loading

jsconan Oct 15, 2024

jsconan Oct 15, 2024

jsconan Oct 15, 2024

jsconan Oct 15, 2024

jsconan Oct 15, 2024

jsconan Oct 15, 2024

github-actions bot commented Oct 21, 2024

		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

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

github-actions bot commented Oct 21, 2024

Version

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