-
Notifications
You must be signed in to change notification settings - Fork 127
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
Split code into separate files #47
Comments
Assigning this to you, @tencircles, let me know if you want a hand! |
I could help on this, if that would be helpful! |
Yes, that'd be great! I think, as a first step, that we should stay away from using a module system (ES6 modules or what have you) and rather just split the files and add a concat npm script or something like that. Moving to ES6 should be on the road map, but I'm not sure we should it all at once. Am I crazy or do you guys agree? @tencircles & @alexanderwallin |
What is your main concern? |
Mainly doing the work in manageable chunks so that we don't get half way through and rage quit. :) Could be, though, that switching to ES6 modules (which I personally prefer over CommonJS) would speed up the process rather than making it more complex? |
My bet's on the latter. :) Why not create a branch, put an eslint config in there and we could start porting little by little? |
Maybe this fella can provide a kick start: https://github.com/lebab/lebab |
@Theodeus I've got some free time over the holiday. I can put in some hours splitting things up. Honestly I was just thinking of doing something like this
what do you think? |
here's a quick PoC https://github.com/Theodeus/tuna/tree/banana |
What is the purpose of the plugin concept? 🍌 |
to modularize the code without adding dependencies. |
So, audio worklets with next version of Chrome, and eventually a deprecation of the script processor. A refactoring has never been more urgent. :) Should we adopt the class based approach used for worklets for everything? https://developers.google.com/web/updates/2017/12/audio-worklet |
Oh, crap! It's been over a year since this thread was active. I was 100% certain it was this December we talked about this. 😮 |
Would it be a good idea to let the actual nodes be the thing that you import? That way you don't get the whole forest when you ask for a banana. Each node imports the core Tuna object and register itself. So in the client code;
In Chorus;
And finally in src/core;
If you just need the Chorus node you'd probably save quite a few kb doing it this way. Whaddoyasay, @tencircles (and how do you feel about using classes)? |
As I'm posting this, I see the hole in my logic.. :) |
Two cents: I don't know the new API, but personally I would like a library like this to provide the tools to register all that myself, rather than solve everything for me. It's much harder to circumvent black box implementations than it is to copy-paste a recipe implementation. |
@alexanderwallin In that sense, do you think it would be better to have a bare-bones core of Tuna that you register the nodes you want to use to, or should it be as it is today, everything included, and if you want to add something extra you just add it to the Tuna prototype? I've been out of js land for a while, so I'm not sure what the best way to go is. My concerns are these;
Additionally, since I haven't spent time with worklets yet, a question I have is if it's supported to use a prototype based approach in creating those? The example I linked above explicitly states that a call to super() is required in an audio worklet (thus requiring a constructor/class approach). Reading the worklet discussions in the workgroup thread kind of enforces that perception. Does anyone know if the same thing can be achieved using prototypes directly? |
|
Supporting both worklets and script processors would be tricky. I don't believe there is full browser support yet. Maybe we could host the worklet scripts on a CDN to save people the hassle of both excluding these files from their normal webpack/rollup build, and making sure they are served somewhere the worklet can access them. We can split out the ScriptProcessor code into separate modules, but I think we may want to hold of on worklet support until there is bigger adoption. Regarding modules. None of the nodes have dependencies on the Tuna core, they really just need access to the user context. I would do something like the following. // nodes/exampleNode.js
import TunaNode from "./TunaNode";
export default context => class ExampleNode extends TunaNode {
constructor (settings) {
this.node1 = context.createGain();
this.x = 2;
}
};
// Tuna.js
import exampleNode from "nodes/exampleNode.js";
import otherNode from "nodes/otherNode.js";
// ...etc
export default class Tuna {
constructor (context) {
this.ExampleNode = exampleNode(context);
this.OtherNode = otherNode(context);
// ... etc
}
} Hopefully the pattern I'm suggesting is clear from the example, let me know if it's unclear. Side note: I would highly recommend using rollup for bundling everything. |
Yeah, maybe I'm jumping the gun a little with worklets. I definitely agree that supporting both will be tricky. I guess the urgency will depend on how soon Chrome decides to deprecate (or, specifically, remove) the script processors. I don't think there's any public indication at all of worklets being implemented in safari, for instance, so we might end up with a need for both eventually. That said, just focusing on modularisation for now is probably a healthy approach. :) |
Your example makes perfect sense, and rollup sounds good to me! I still think it'd be a cleaner pattern if the nodes could register themselves with Tuna, so that we don't have to import and initialise each node type explicitly in Tuna.js. I kinda like it how it is today, that the nodes simply extend the Tuna prototype. There should be an include-all-nodes version, just like tuna-min.js today, which is the main way to consume tuna (pun not intended). But we should also include instructions on how to build Tuna from scratch, to allow those that want to shave kilobytes of the library to do so. I think that premise could help drive the choice of architecture. Do we want people, when building, to go into Tuna.js and remove the imports/init for the node types they don't need. Or do we want them to physically remove the individual node files to exclude them from the build? I would assume that the tree shaking in rollup isn't clever enough to know that, for example, the Chorus node isn't used in an application using Tuna, if we import the Chorus in Tuna.js, right? If it is, then your approach sounds like the best way to go, and you can ignore the paragraph above. So many questions. I should really try things out and experiment instead of expecting you to know everything. :) |
@Theodeus take a look in the |
Answering inline on the commit, but for visibility - I think I like it! |
@Theodeus I've been working on this PR. Part of the reason it's taking so long is the AudioParam mock. I think this is quite necessary actually, but it requires a bit of legwork to implement. I'll hopefully have an update on this soon. |
Sounds awesome! I'd be happy to assist if you need a second pair of eyes/hands. :) |
@tencircles How is your progress? |
I've rewritten this library in Typescript using es6 classes and made each node tree-shakable: #94 |
No description provided.
The text was updated successfully, but these errors were encountered: