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

initial replacements of module system #489

Merged
merged 12 commits into from
Jun 28, 2020

Conversation

endurance21
Copy link
Collaborator

@endurance21 endurance21 commented May 13, 2020

This Pr is under the purview of GSOC'20 project

  • all native require and module.exports of nodejs has been changed to es6 export and import in src folder !

@LilaShiba
Copy link
Collaborator

@endurance21
Copy link
Collaborator Author

i have added p5.js file for the sole purpose of testing the APIs !

@endurance21
Copy link
Collaborator Author

@therewasaguy @kyle1james
fixed the bug of setting the context of tone.js explicitly!
looking forward for your reviews !

Copy link
Collaborator

@LilaShiba LilaShiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two conflicting files, which should be a quick fix. The PR highlights those for you. I'm having trouble verifying a PR due to lack of tests in place. So, I'd wait for Jason to give that ole 👍

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@endurance21 can you highlight any changes you've made to the src files, aside from changing import/exports? It's hard to know what the diffs are since the indentation of every file changes now that we're not wrapping everything in a define function.

  • The compiled lib/p5.sound.js seems to have shrunk significantly. Have you rebuilt it with the latest changes? In the future, it would be easier to review the changes to src/ files separately from the compiled library.

  • The Tone API has changed, and instead of a TimelineSignal, we're now importing the TransportTimelineSignal. This signal is very similar to the Web Audio API's AudioParam https://developer.mozilla.org/en-US/docs/Web/API/AudioParam, but it enables us to getValueAtTime which is not part of the web audio API spec. It is used in p5.Envelope and p5.PolySynth. And, in order to use it, we need to "start" the Tone Transport.. We can do this with

import Transport from 'Tone/core/Transport';
Transport.start();

package.json Outdated Show resolved Hide resolved
src/amplitude.js Outdated Show resolved Hide resolved
src/audiocontext.js Show resolved Hide resolved
src/envelope.js Outdated Show resolved Hide resolved
src/polysynth.js Show resolved Hide resolved
src/polysynth.js Show resolved Hide resolved
src/polysynth.js Show resolved Hide resolved
@endurance21
Copy link
Collaborator Author

@therewasaguy i have made the requested changes and resolved the merge conflict too !
looking forward for the merge !

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like oscillator has not been updated, and it's throwing some errors when testing locally.

also, be sure to either update the compiled library, or exclude it from the PR/commits

lib/p5.sound.min.js Show resolved Hide resolved
@endurance21
Copy link
Collaborator Author

it looks like oscillator has not been updated, and it's throwing some errors when testing locally.

also, be sure to either update the compiled library, or exclude it from the PR/commits

added the oscillator updates !

Copy link
Collaborator Author

@endurance21 endurance21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two conflicting files, which should be a quick fix. The PR highlights those for you. I'm having trouble verifying a PR due to lack of tests in place. So, I'd wait for Jason to give that ole

done !

src/audiocontext.js Outdated Show resolved Hide resolved
@endurance21
Copy link
Collaborator Author

@therewasaguy i have not pushed the latest build file so , while you are running tests locally using my latest commit , make sure to build it once !

src/effect.js Outdated Show resolved Hide resolved
Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good!

I would love to see it go a step further to ensure consistency with how we export and import modules, as mentioned here, but that doesn’t need to happen as part of this PR.

Just remove a comment, rebuild (or remove the builds), and we can merge.

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @endurance21 sorry I was unclear about this in my previous comment, but currently, there's a commit that adds “deletes” for the /lib files to the commit history. I don’t think we want that, but rather to remove the modifications to those files from the commit history. Something like this...

Or, it's probably easier just to include the compiled lib in this PR, if you want to add it back :)

src/oscillator.js Outdated Show resolved Hide resolved
@endurance21
Copy link
Collaborator Author

@therewasaguy also there was p5. Js file added in that folder only for testing purpose,

So finally i think i have to do is

1.remove the p5. Js file from lib
2.build using grunt and commit those build file
??

@therewasaguy
Copy link
Member

yes, that sounds good @endurance21

@endurance21
Copy link
Collaborator Author

@therewasaguy i have added build files , take a look at that !

Copy link
Member

@therewasaguy therewasaguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great work, @endurance21. Let's ✅ this massive PR! :shipit:

@endurance21 endurance21 requested a review from LilaShiba June 27, 2020 06:34
Copy link
Collaborator

@LilaShiba LilaShiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@therewasaguy
Copy link
Member

@endurance21 feel free to "squash-and-merge"

@endurance21 endurance21 merged commit d32113c into processing:master Jun 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants