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.

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

Find a work around to address RequireJS AMD related problems in production #2632

Open
meganindya opened this issue Nov 28, 2020 · 15 comments
Open
Assignees
Labels
Issue-Bug Priority High Architecture Priority in refactoring the program architecture Priority-High Serious bug regarding performance

Comments

@meganindya
Copy link
Member

meganindya commented Nov 28, 2020

Here below is a screenshot of the production release at musicblocks.sugarlabs.org. The application crashed at loading. However, it loaded on refresh, albeit with a few errors and warnings again. I have had this issue several times before, on multiple devices especially mobile ones. This is a bad thing!

Screenshot (19)

Most of the stuff here looks like an issue with the asynchronous loading via RequireJS, which actually would mostly happen when loading from a remote server, so these things aren't often reproduced during local development. #2629 is therefore a priority.

#2614, #2615, #2618 address the problem. Crash errors are the priority.

@meganindya meganindya added Issue-Bug Priority-High Serious bug regarding performance Priority High Architecture Priority in refactoring the program architecture and removed Priority-High Serious bug regarding performance Issue-Bug labels Nov 28, 2020
@meganindya meganindya reopened this Jan 23, 2021
@meganindya meganindya changed the title Investigate console errors in production Find a work around to address RequireJS AMD related console errors in production Jan 23, 2021
@meganindya meganindya changed the title Find a work around to address RequireJS AMD related console errors in production Find a work around to address RequireJS AMD related problems in production Jan 23, 2021
@meganindya
Copy link
Member Author

meganindya commented Jan 23, 2021

Investigation into this problem concludes that there is a significant problem in js/activity.js. Although all the code has been relevantly split across several files, the codebase isn't effectively modularized. There are some GOD objects like Logo (js/logo.js), Blocks (js/blocks.js), Turtles (js/turtles.js), etc. which do a lot of heavy lifting and maintain application state. These are all instantiated in js/activity.js. Many files actively reference these objects as they are effectively global.

There is a significant issue with the module architecture. Only one file, js/activity.js, loads all the files as AMD modules via Require JS. import and export have not been used as browsers had support issues, and bundling hasn't been implemented in Music Blocks.

define(MYDEFINES, function (compatibility) {

The above call imports all files defined in MYDEFINES
MYDEFINES = [

The problem with this is that the code effectively becomes a monolith — the behavior is as if the code of each file has been appended one after the other into one large file. Even if we let go of actual modularization and use the development tools in the browser, a problem still persists. Require JS is based on Asynchronous Module Definition (AMD); files/modules are loaded asynchronously. Now, JavaScript isn't loaded; the files are parsed into ESTree-spec Abstract Syntax Trees and nodes are loaded asynchronously. The order is subject to the availability of the entire file. Suppose, there are two files that need to be loaded one after the other in order, but, say the latter file is smaller in size and is downloaded before the prior, the browser will attempt to load the nodes of the file which is available first. If there are top-level references of some constructs of the prior file in the latter, the loader crashes. Again, if both files are available, the browser need not necessarily load all nodes of the prior file before those of the latter. The order can be practically arbitrary owing to several factors, and so, if some reference is encountered before the definition, the loader crashes.


Now, fixing all of these effectively by reorganization is a gargantuan task. Ideally, each module should import its dependencies but, it is extremely cumbersome here. There could be circular dependencies too. Probably, there's no easy solution to the global import. But, we could perhaps trade-off some load speed for reliability.

As of January 2021, there is sufficient support of import/export. Using await import on some of the more crucial file could solve the reliability problem. The rest are mostly independent and can be asynchronously imported. But, this is mere speculation. However, the dependencies need to be chalked out properly. #2609 helps with this identification.

There's more to it though. The development history of Music Blocks dates back some years to quite a simpler application from which it was forked. Over time it has been bloated and unfortunately no build step was involved down the line. This means that the application is very bulky. All files are hosted on the server as they appear in this repository, which is not a fair practice in modern times. I think bundling is inevitable.

I am assigning myself to this sub-project. I'll be configuring webpack, review and lint all the files, chalk out a dependency structure, and create a build strategy. Yet, then the application will use a lot of globals, which is anti-pattern, but I guess we'll have to settle with that for now.

@endurance21
Copy link

@meganindya
Is it possible to load modules synchronously using require.js despite its used AMD module loader ?, mostly i would deny with this.
I think the best would be to have a build system using browserify or webpack and modularise code with export and import thing ! , It will highly make the codebase ,modular and intuitive to developers out there.

@meganindya
Copy link
Member Author

Not require.js. I suppose the new import syntax would work. This application has a global pattern with GOD objects. I'm not sure if it is practically worth the effort modularizing this since there are circular dependencies. MB is being rebuilt; this issue is only a wrap up move.

@endurance21
Copy link

if you are refereing to ES6 export and import feature of browsers, one thing we should not forget is that, in this case the js files has to be served using a server !
export/import does not work elseway , if we are not using any build system or server to serve our js file`

i am not sure if we are using any static or any kind of server to serve our js file to browser or we simply download all the js file once to browser ?

@endurance21
Copy link

@meganindya also i am not aware of the term GOD , it would great if you would elaborate about it ?
thanks

@meganindya
Copy link
Member Author

They're objects that do a little too much of work. They're a common anti pattern. We unfortunately have a few of those. Logo, Blocks, Turtles are our god objects. They store too much state information and handle a lot of manipulation. Even objects of Painter and Singer are heavy in state information. Plus we have global constants and utility functions distributed all around. All these add up.

@meganindya
Copy link
Member Author

Perhaps, my words were cryptic and inconsistent. I've reworded the description. I've assigned myself to this. Follow the progress Webpack bundling and building.

@endurance21
Copy link

@meganindya I am sorry about the reply , I just wrote it down and almost forgot to comment.

It makes sense and I can get to know more as I go along.

@endurance21
Copy link

@meganindya I would be very happy to help you out.
Can you tell me where to get started.?

@meganindya
Copy link
Member Author

Please use the project link shared above to follow my plans. Since this application will be archived soon. I want to get things sorted in the minimum effort, even if that isn't the best approach in the coding sense. All the progress will be merged from sub-feature branches onto the feature branch 2632-bundling.

I don't want to go too deep into refactoring. The application relies on some of globals — anti-pattern 'GOD' objects, utility and glue functions, and constants. Ideally, I would want a total hierarchical module architecture, but I suppose that'll be a lot of effort, which is perhaps not worth applying. I am considering settling on the globals but in a more reliable manner.

Most (almost all) of the global objects are initialized in js/activity.js. index.html references most of the libraries. Global utility (and glue) functions and most constants are present in the source files in js/utils/ directory. Rest are sparingly distributed across the rest of the source files in js/ directory. Referencing of image (and other file type) resources are highly inconsistent; a lot of DOM manipulation is done directly in the source files in a very dirty manner (using innerHTML property). And, a lot of the image resources are referenced directly from the repository in other places like README files, documentation, guide, and Wiki (I moved some of them in #2834 from markdown files). There is a lot to take care of. As I mentioned, I am not interested in investing effort into an ideal structure — Music Blocks' poor architecture is due to its development history; Music Blocks 2.0 will have an elegant architecture. The job right now is just to make this application stable.

My rough plans are as:

  • Initial cleanup and reorganization of source files and resources (completed in Initial cleanup #2834)
  • Reorganize global constants
  • Reorganize global utility functions
  • Reorganize other glue functions
  • Refactor js/activity.js to be cleaner and consistent
  • Refactor global objects' initialization in js/activity.js more consistently
  • Configure export in the files that share globals
  • Configure import of globals and other module nodes in rest of the files (in progress linting work will help identify all external requirements of each module)
  • Configure Webpack to 'file-load' the resources (images better remain global rather than being importable resources in source files)
  • Bundle the module type source files (js/activity.js is not bundled) with Webpack
  • Insert the bundles and other scripts in a slightly modified index.html template with Webpack
  • Configure miscellaneous features in Webpack like optimizing building, hot-load, etc.

I suppose there will be multiple bundles. Perhaps, js/activity.js will go last instead, but we'll see to that later. These is a speculative plan. Some of these might change as required, but this is something I want more or less. I'll create cards to the To-do column of the project as I go. I'll roughly describe the individual plans.

Add a notifying comment here if you pick something to work on. After discussions, when you begin working, create a sub-feature branch forked from the HEAD of the 2632-bundling feature branch. Pull requests will be made to merge into 2632-bundling.

@daksh4469
Copy link
Member

Hi @meganindya, I would like to help with this. Where should I start?

@abhishekkumar08
Copy link
Contributor

Hello @meganindya @daksh4469 I would also like to work on this issue. Mind if we could share the work together as I think big changes have to be made for this.

@meganindya
Copy link
Member Author

I can't really commit to this right now. I'm busy planning for MB 2.0. Please suggest exhaustive progression steps for this by adding a comment here. My plans listed above are rough ideas. It can surely deviate from that.

@sksum sksum unpinned this issue Mar 25, 2021
@sksum sksum pinned this issue Mar 25, 2021
@ChiragJS
Copy link
Contributor

ChiragJS commented Jan 3, 2024

Hey ,is this issue still supposed to be worked on?

@AdityyaX
Copy link
Contributor

AdityyaX commented Mar 1, 2024

Hey, @ChiragJS Is there something up for grabs in this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue-Bug Priority High Architecture Priority in refactoring the program architecture Priority-High Serious bug regarding performance
Projects
None yet
Development

No branches or pull requests

6 participants