Refactor: Use ES6 modules and add bundler

[daviddostal]

Solves most of issue #77.

Hello, I finally finished work on adding a proper module system, here is the result of refactoring to use ES6 modules + adding Rollup to do the bundling. For now, only the files in /javascripts are bundled, bundling of library dependencies can be eventually added later.

I decided not to produce a separate beak.js bundle, but instead let Rollup figure out the common code between multiple pages and automatically split it into separate chunks. IMO the automated splitting makes it easier to maintain and we don't have to create a separate entrypoint file for /beak, which just imports and reexports everything from beak.

Below is a list of changes and an explanation of how the bundling works. Let me know if you have any remarks or questions.

List of changes:

1. Use exports and imports instead of global variables.

2. Remove circular dependencies between files:

   Rollup (and most other bundlers too) show a warning on circular dependencies between files, because it has no way of knowing the order in which the mutually dependent files should be loaded.

   (It seems nothing breaks even with the circular dependencies, but I decided to refactor and remove circular imports anyway, so we can 1. avoid the annoying warning and any potential surprise issues and 2. it solves an issue with tests at the same time - see change no. 3.)

   Below are the detected circular dependencies and how they were eliminated:

   • widget.coffee ⇒ draggable.coffee ⇒ context-menu.coffee ⇒ widget.coffee:

     Split part of context-menu.coffee and create separate contextable.coffee file so draggable.coffee doesn't import context-menu.coffee anymore.

   • tortoise.coffee ⇒ session-lite.coffee ⇒ tortoise.coffee:

     Pass Tortoise to session-lite as constructor parameter to avoid circular import.

   • tortoise.coffee ⇒ session-lite.coffee ⇒ initialize-ui.coffee ⇒ config-shims.coffee ⇒ tortoise.coffee:

     Split out utility code from tortoise.coffee into separate file tortoise-utils.coffee, which is then imported where needed. This means config-shims.coffee doesn't need to import the entire tortoise.coffee anymore, only the tortoise-utils.coffee.

3. Refactor tests to import tortoise-utils.coffee instead of tortoise.coffee.
   This at the same time also avoids issues, where tortoise.coffee had dependencies on other files (such as tortoise-engine.js), which weren't easily accessible at test time.

4. Move javascript code from .scala.html views into separate js files in the /javascripts/pages directory, so they can become entry points for bundling (inline scripts in .scala.html files cannot be easily bundled).

5. Changes required by forced strict mode in modules. For example arguments.callee is no longer available; we cannot change this using .call on a function; variables are locally scoped by default. Files affected by the first 2 issues are:

   • draggable.coffee (avoid arguments.callee, add additional ractive parameter replacing this)
   • resizer.coffee, edit-form.coffee, async-user-dialog.coffee (properly call draggable)

6. Add Rollup for bundling (and minification in production): This consists of integrating Rollup in build.sbt and the Rollup configuration in rollup.config.js. More on this in the section How the bundling works below.

7. Remove many script references in views, which aren't necessary anymore, because import statements do the job now.

8. Add support for additional attributes in TagBuilder.scala to allow setting type="module" on scripts.

9. Updates to extraHead of /differences page in Application.scala to account for changes in script locations.

10. Detect if /simulation is standalone and include full bundle of everything (without splitting out common code into separate chunks), when it is standalone.

11. Access markdown.toHTML directly on window. We can access markdown.toHTML directly on window from widgets/ractives/info.coffee. It isn't used anywhere else, so I don't think it needs to be assigned to a global exports variable anymore.

12. Remove unused method CompilerService.scala#tortoiseLiteJsUrls. Since commit 1393311a3, there are no references to the method. At first I was trying to update the asset URLs, but thankfully tortoiseLiteJsUrls is probably no longer needed.

How the bundling works

The SBT configuration

The bundler is run as part of the sbt-web asset pipeline using a custom pipelineStage called bundle. The bundle stage currently runs after the sbt-coffeescript plugin and takes the compiled javascript files as its input. To do the bundling, we run a Rollup process from SBT with arguments specifying the Rollup config file and input and output folders for the bundler.

(Because pipeline stages only get a map of input files from potentially different locations and Rollup needs all its inputs in a folder, we also need to sync the files to a temporary folder, from which Rollup reads them. This technique is also used for example by the sbt-rjs plugin and there even is a nice built-in helper method in sbt-web.)

Other pipelineStages can be run before or after bundling, for example digest to generate fingerprint hashes in production. A playRunHook is used to detect when the app is running in development mode, so we can output source maps only in dev and minify scripts only for production.

Rollup config

The bundler configuration is specified in rollup.config.js. The configuration file exports a function, which takes arguments with which Rollup was run (such as the source and target directories) as parameters and returns an array of bundle definitions (simple JS objects, which specify input and output options, required plugins etc). Because the config is just a javascript file, we can for example define our own helper functions for generating the configuration, or use the Nodejs api.

The bundler configuration for this project has 3 main parts:

1. Entry points in the /pages directory:

   All scripts in the javascripts/pages directory are considered entry points for bundling. Because we list the files from that folder using the Node fs API, more entry points can be simply added to the pages directory as a .js or .coffee file in the future and no other work is needed.

   Common code between the pages is automatically detected by Rollup and split into separate chunks (and then imported from the compiled entry points). We could also create a bundle of common code (for example /beak?) manually, but IMO it's simpler and less prone to break on changes to just let Rollup figure it out.

2. Standalone bundle of /simulation:

   When the /simulation page is rendered in standalone mode for exporting as a HTML file, all scripts need to be directly embedded in the page (no imports, script src="..." etc). This is why we generate a separate full bundle of pages/simulation.js, which contains everything in one file and does not split out and import any separate chunks with common code.

3. Scripts imported by tests:

   Because I found no way to skip the bundling stage when running tests (sbt always runs the asset pipeline before even compiling tests), the bundler also needs to output any javascripts imported from tests (for now it is just beak/tortoise-utils.js). We also output the files needed for tests in the commonjs format, so they can be simply require()d by the tests running on Node.

Possible future improvements

In this refactor I focused on getting the basics working. Some possible improvements for the future, which are not included in this refactor:

• Bundling of libraries (Ractive etc.): In the future we might also import some of the libraries and either produce a separate bundle of libraries or bundle them with the script files.

• Absolute import paths: For now all import paths are relative, which sometimes causes a bit of ../../. Although this isn't a huge issue, in the future we might refactor some imports to absolute paths from the root or even consider using @rollup/plugin-alias to alias different import roots, for example import "@ractives/..." instead of import "javascripts/beak/widgets/ractives/...".

• Allow .coffee extension for imports: Currently all import statements need to use the .js extension, even when importing a .coffee file. This is because behind the scenes, sbt-coffeescript compiles all .coffee files to .js, but does not change any file extensions in import statements. And the bundler doesn't know about any previous coffeescript compilation and resolves imports relative to the already compiled files. This could be improved either by replacing sbt-coffeescript with a Rollup coffeescript plugin or maybe by writing a simple custom resolver for Rollup, which replaces '.coffee' with '.js' when looking for imported files.

• Further reduce the amount of global variables: There are still many globals, although their number is now reduced by more than 1/2. Most of the remaining global variables are from libraries (could be solved by importing some of them instead) and from the Tortoise runtime (this would require changes to the Tortoise compiler).

Things I might have missed

I tried to be diligent and test everything during and after the refactor, but it is possible that something still slipped through. Here are 2 areas, where I feel a bit less confident, so you can keep that in mind during code review:

• I don't yet fully understand, how extensions work in NLW, so I might have accidentally removed a global variable needed by extensions. For exampleconfig-shims.coffee#genConfigs comes to mind here, should that remain global?

• Although I don't think I did, I might have missed something during rebasing on top of the latest changes, because there were quite a few merge conflicts I had to resolve.

---

PS: I hope you don't mind that I squashed all changes into one commit, I did that to avoid even more merge conflicts during rebasing.

If there are any changes you'd like me to make, let me know!

[LaCuneta]

This is awesome. It obviously took a lot of effort, and I appreciate it. It feels good seeing so many things ripped off the window and getting rid of circular references is a big bonus.

For making a general beak.js - part of the reason there is so other projects could consume the front-end functionality from this project, especially if it was published to a package registry. That is much more of a "nice to have" feature which should be much easier to implement with these changes, so I do not think it needs to be done to merge this PR.

A few style issues (we should really lint these):

• Some files did not end in blank lines. We prefer a blank line at the end of each file.
• The new Javascript files have four-space indents, we'd prefere two-spaces.
• We usually add name and date to comments, to avoid having to search the git logs more than necessary. The detailed comments in the build.sbt file are great.

I found a couple of issues while testing:

The exported standalone NetTango Web model HTML pages are trying to reference the NetTango builder JS file as an external script. That should also be inline so the HTML file can function stand-alone. You can try it out from http://localhost:9000/nettango-builder > Files menu in the middle of the page > Export standalone HTML file.

Trying to export the standalone NetLogo HTML file from NetTango Web threw a 404 error. Again, from the /nettango-builder, click the HTML button in the NetLogo model's Export: area.

And a couple of issues with debugging:

I noticed that the individual script files are no longer presented as individuals when doing development. So if I open the web developer console in Chrome with a model running, I cannot do a file search for session and jump to the session-lite.js file to set a breakpoint at a spot I know. I can just find and open the chunk file then search for SessionLite in there, or do a global search for the same. I was just very used to do doing a quick Ctrl-P and then opening the file directly. Is there any easy way to get individual script files generated while running in development mode?

Relatedly, the file containing the actual Beak view code is named new-model-########.chunk.js. Is there a way to get that chunk file a more descriptive name?

The genConfigs changes seem fine. I tested out a couple extensions and things were good. Other things that depend on the config shims like mouse actions, image export/import and the rest were also fine.

[TheBizzle]

🎉 Wow! 🎉

🙇‍♂️ Thank you, thank you, thank you, thank you, thank you for submitting this enormous and incredibly valuable pull request! 🙇‍♂️

I have an immense amount of respect and appreciation for going through all the trouble to do this, and doing things that I should have done years ago, myself. This is so exciting, and makes the technical foundations of this project feel so much more legitimate. Thank you!

Honestly, looking this over, while I have a number of small quibbles, I think this PR is of incredibly high quality. At nearly every important crossroads, you made what I consider to be the most intelligent refactoring decision. I find it really impressive! 👏

2. Remove circular dependencies between files

Awesome, thank you! Great choice!

Possible future improvements: Absolute import paths

I do favor the absolute paths, if, for nothing else, making it clear to the programmer, at a glance, where a find an imported file in the file structure. Computers are a lot better (and faster) than me at figuring out what specific file "../../../widget.js" is referring to.

It would also make for safer refactors in most situations (and more-unsafe ones in other situations, like when moving an entire folder).

Possible future improvements: Bundling of libraries (Ractive etc.)

What's the advantage of this?

Possible future improvements: Allow .coffee extension for imports

What's the advantage of this?

I might have accidentally removed a global variable needed by extensions. For example config-shims.coffee#genConfigs comes to mind here, should that remain global?

That code looks correct to me. Nothing to worry about there!

Again: Thanks, a trillion times over, for all your work on this. I'm looking forward to seeing it all get merged in before long, and released out into the wild on netlogoweb.org ! 🎉 🎆 🎉

[daviddostal]

Thank you @LaCuneta for the thorough code review and catching some bugs and issues!

I updated the pull-request by adding a new commit with the requested changes.

1. General beak.js bundle:

   Yeah, that makes sense. Because it's more of a nice-to have, I'll leave that for another time. Here is what I think would need to be done:

   • Think about what exactly the public API of beak should be.
   • Make a beak.coffee file, which imports and reexports stuff we want to expose from the library.
   • And then simply add beak.coffee as an entry in rollup.config.js (maybe only for production builds?).
   • Possibly change imports in other files to import directly from the public API in beak.coffee and not other files in the /beak directory.

   I don't know what exactly should be exported from a beak bundle, so this is probably on you guys, but I'm glad to help with the rollup config if you have any questions, because I'm already quite familiar with Rollup.

2. Blank lines at end of files:

   Fixed ✔. I also changed coffeelint.json to check for this issue (I decided to use the "error" level, because that is what was used on the somewhat related line_endings rule.)

3. 4-space indents in new js files: Also fixed.

4. Add name and date to comments:

   Name & date are added to most comments. Generally I observed from the codebase, that names & dates are mostly added to comments explaining why something is the way it is, but not to simple comments explaining what a piece of code does. I hope that is what you had in mind.

   While I was at it, I also edited a few comments to be more clear and useful.

5. Broken HTML export of NetTango model:

   Thank you for catching this, I totally missed that. Should be fixed now.

   On localhost the exported model shows a warning on load: Unable to load the NetTango library file. Are you running on netlogoweb.org?. The warning was also present in commits before the refactor, so I assume this is expected.

6. Broken Export > HTML button on /nettango-builder:
   This was already broken before the refactor, but I fixed anyway, so that should work now too.

7. Using Ctrl+P in Chrome devtools:
   I modified the Rollup config so scripts aren't bundled in development mode. Now not just the mapped .coffee files, but also the compiled .js files should be accessible using Ctrl+P.

8. Confusing names of chunk files:

   You are absolutely right the generated file names are a bit confusing (although it isn't a problem in development now after the previous fix).

   Giving the chunks more descriptive names is impossible, because common code is automatically split by Rollup and we shouldn't really care, how the code is split.

   But it is possible, to make the names of generated chunks less confusing by changing them from [name]-[hash].chunk.js to [hash].chunk.js. Now it should be more obvious, that the names have no important meaning.

Again, thank you very much for taking the time and reviewing such a large amount of changes!

[daviddostal]

@TheBizzle it makes me really happy to hear you like the pull-request and the choices I made 😊. I'm also looking forward to seeing the changes become part of this project. For some choices it took exploring nearly every wrong way to find the right one, but I'm glad it turned out well in the end.

1. Absolute import paths:

   Good point, absolute imports certainly are a lot more readable than ../../../.

   It would be nice, if all absolute imports don't have to start from the project root with /app/assets/javascripts/..., so I wrote a very simple Rollup resolver, that allows specifying a base directory for absolute paths. (It took just a few lines of code, I love how flexible Rollup is with this kind of stuff! See function absoluteImportBasePlugin in rollup.config.js.).

   For now I changed all imports that contained one or multiple ../s to use absolute paths. I didn't change paths to files in the current directory (./foo.js) or in subdirectories (./bar/baz.js), because I wasn't sure, if you wanted to make all paths absolute or just get rid of ../s. Changing all paths to absolute would make import paths in deeper nested folders such as /beak/widgets/ractives/subcomponent a bit long and moving/renaming directories would be a bit harder, but it would also make refactorings such as moving files a bit easier.

   ❓ Should I make all imports absolute? If so, just let me know and I'll make the changes.

   (There is also the possibility of using @rollup/plugin-alias to alias certain directories and make long paths shorter. An import statement could look something like import foo from "@ractives/subcomponent/foo.js", . It would also make moving folders easier (just change where the alias points to.)

2. Advantage of bundling libraries:

   I don't know, if there is a big advantage for bundling libraries. I included this, because it was mentioned in the original issue:

   3. Produce a dependency bundle as well so we can host/link that instead of each individual dependency script.

   It might be nice to import some of the libraries to get rid of some global variables, like you mentioned for makrdown-js.

3. Advantage of .coffee; extension for imports:

   One small advantage of using the .coffee extension for importing .coffee files would be, that it would arguably be a bit more intuitive, because the imports directly point to a known file in /assets, and not some file, that will be created in the future by the build process.

   Besides that, my thought was it would make some editors understand the import statements and allow for quicker navigation between files. But I don't think that still holds when using absolute imports with a base directory other than the project root.

Thank you for reviewing the code!

[LaCuneta]

I re-ran through the same tests I did before and everything is resolved 👍

I also got these changes published to our experiments site, since I wanted to confirm everything went through our scrape and publish process normally. Everything went well with it (issues with the deploy script being out-of-date aside).

I noticed one method signature comment change needed in session-lite.coffee for adding the Tortoise parameter. I also saw that simulation.js and mainTheme.js are missing the line break before the end of file. Those obviously didn't get caught by the CoffeeScript linter setting you updated (thanks for that!).

I found one more issue, but I'm struggling to see how it's related to these changes. It appears that color widgets are no longer binding their values to the model, they just stay set to black no matter what. To see the issue you can check out CA 1D on the experiments site. I confirmed the same behavior running locally, and also confirmed that things are working in staging, so it does seem that somehow the changes here are causing the break. I thought this might be related to the workaround I had for Safari's lack of support for color inputs, but in checking that code is not run (at least, for me on Chrome). Unless somehow just loading that library is causing the break? If necessary, that workaround is no longer needed as Safari supports color inputs, so that library and code can be removed if that makes a fix easier. If it's not related, I'll just remove it after this is merged.

Otherwise, I think this is very close to merging! Once more, thank you so much for your work.

[TheBizzle]

Should I make all imports absolute?

While I had originally been suggesting that, I feel less strongly about that now, considering that all of the parent paths (../) have been removed. Sibling and child paths are easy enough to read and reason about, I think.

Thanks for taking care of that! 😎 👍

[daviddostal]

@LaCuneta

I re-ran through the same tests I did before and everything is resolved +1

I also got these changes published to our experiments site, since I wanted to confirm everything went through our scrape and publish process normally. Everything went well with it (issues with the deploy script being out-of-date aside).

That is great, I'm glad to hear everything went well!

(Speaking about deployment, I noticed modelslib/, nt-modelslib/ and chosen-sprite.png is being copied to the target directory on deployment. If I understand it correctly, the reason for this is the SBT digest stage changes the filenames, but asset versioning cannot be used for these, so we copy the original files. If this is the case, there is a option to exclude files from digest in a similar way like I did for *.chunk.js files in build.sbt:162. Just wanted to let you know in case that is useful...)

I noticed one method signature comment change needed in session-lite.coffee for adding the Tortoise parameter. I also saw that simulation.js and mainTheme.js are missing the line break before the end of file. Those obviously didn't get caught by the CoffeeScript linter setting you updated (thanks for that!).

Thank you for noticing that and sorry for missing these newlines again.

It might be worth it to convert all .js files to .coffee files in the future, so all scripts can be linted. But it's more of a maybe-nice-to-have-someday and I didn't want to change it as part of this refactor.

Broken color input
The fix was surprisingly simple, the issue was just a missing import. The error was silenced due to a catch-all try/catch, so that's why it wasn't immediately obvious. I'm really glad it wasn't something more obscure and hard-to-debug 😌.

To be sure I also checked all other try/catch statements for this issue and they looked fine.

[LaCuneta]

(Speaking about deployment, I noticed modelslib/, nt-modelslib/ and chosen-sprite.png is being copied to the target directory on deployment. If I understand it correctly, the reason for this is the SBT digest stage changes the filenames, but asset versioning cannot be used for these, so we copy the original files. If this is the case, there is a option to exclude files from digest in a similar way like I did for *.chunk.js files in build.sbt:162. Just wanted to let you know in case that is useful...)

Thanks for the tip, that is useful, I'll make a note to look into that change. My mental model of the digest system is definitely lacking.

I finished up one last round of tests and code review and everything looks good to me.

[TheBizzle]

🎉 LGTM! 🎉

A million thanks for all your work on this! 😎 👍