Split contents of dom.js among four(4) files

[SilasVM]

Resolves #7308

Changes:

Distributed the code within dom.js, across dom.js, p5.Element.js, p5.MediaElements.js, and p5.File.js.
Specific details of the split are detailed in Proposal #7308.

I've had trouble running the npm test command. I was unable to run it on a clean branch, so I don't think this is a direct indication of any issues with my changes. Please let me know if there's an alternate form of testing for this branch

I was also unsure of how to modify lines 13-19 of each file.

• Should dom.js import each new file?
• Do lines 13-16 need modification?

Screenshots of the change:

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

Please let me know your thoughts @limzykenneth.

[davepagurek]

Hey @SilasVM, what errors did you get when running npm run test?

[davepagurek]

Do lines 13-16 need modification?

What you've got now looks good to me!

Should dom.js import each new file?

Currently the dom module is imported here, which we'll need to modify:

p5.js/src/app.js

Lines 42 to 43 in 39aab83

	// DOM
	import './dom/dom';

Other modules that we've refactored now export a function that gets called to set it up, e.g.:

p5.js/src/app.js

Lines 39 to 40 in 39aab83

	import data from './data';
	data(p5);

So we'll probably want to do that here too, following a similar pattern to other modules, which register an addon in their exported setup function, and then structure the implementation files like addons where we add properties to the fn parameter as a shortcut for adding to p5.prototype.

[SilasVM]

Hey @SilasVM, what errors did you get when running npm run test?

Thank you for taking a look and giving me the suggestion to modify app.js @davepagurek!

Currently I'm getting a lot of errors surrounding the DOM module. Some are saying "should be function" and it seems some of the methods aren't returning output as they should.

I modified app.js which reduced the amount of errors I was getting.] from 7 files and 93 failed tests to 5 files and 48 tests failed.

When running npm test on a clean fork of dev-2.0 I'm seeing 4 files and 10 test failed(I'd appreciate it if someone could test this as well to see if these failed tests are a result of a lacking dependency or some error occurring on my device):

This is my output when running it on my pr fork after updating app.js, and the first screenshot in this comment is a more detailed look into what's failing:

I'm going to continue testing things out by modifying app.js, but I wanted to at least share what I've got so far.

[SilasVM]

Giving another update. I believe I resolved many of the issues. I implemented the index.js file within the dom folder. I modeled the files off of the data folder you mentioned.

My npm test now gives results equal to that of when I ran it on a clean fork. I'm still not sure if these errors I'm getting from the clean fork are a result of my device or not though.:

I'm now getting this issue when attempting to commit however:

[limzykenneth]

@SilasVM For the linter error, please run npm run docs first and that should clear it. I'll check the tests.

[SilasVM]

Thank you @limzykenneth, the error was cleared.

npm test now displays all clear tests within the DOM module:

These are the remaining error spots from npm test:

More detailed looks at the first 3 files:

The 4th file:

[limzykenneth]

CI tests are passing so that's fine for now, those failing are visual tests so probably related to some threshold we can tweak later.

[SilasVM]

@limzykenneth, when I was implementing your requests, I noticed the following files have this unnecessary line of code: import p5 from '../core/main';.

I removed the line in each of the 4 files, ran npm test, and everything in DOM passes as it should with no new test failures. I have the branch ready to push, would you advise leaving this line in any of the files for any reason or should I go ahead and update the PR with this modification?

[limzykenneth]

Yes these are not typically necessary with the new module syntax.

[limzykenneth]

@SilasVM Looks good. Thanks!

[SilasVM]

Thank you @limzykenneth and @Qianqianye for this opportunity and your assistance throughout!