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

First working port to Rollup #499

Merged
merged 6 commits into from
Oct 29, 2019
Merged

First working port to Rollup #499

merged 6 commits into from
Oct 29, 2019

Conversation

Symbitic
Copy link
Contributor

I got my first working port to Rollup ready. Gulp has been completely removed and is no longer necessary.

Some advantages include:

  • Real ES6 modules
  • Smaller build sizes
  • When my Rollup port for Brain.js is working, it will be able to bundle GPU.js directly. Babel usually skips transpiling anything in node_modules, so both should be usable in projects that use Babel.
  • The dist folder could be added to .gitignore now. NPM automatically runs npm run build, so those files will be built anyway.

That being said, there are some things to look out for:

  • I had to HEAVILY refactor almost every file to use ES6 Modules instead of CommonJS. Speaking of which:
    • Circular dependencies are an abomination from Hell! I had to refactor a lot of files to remove it. I also had to remove the two functions in utils.js that used the GPU class. IT JUST COULDNT BE DONE! Besides, it didn't look like anyone or anything was actually using them. Plenty of other refactors were necessary.
  • HeadlessGL is not exported in browser builds. It was just too much effort, it resulted in big bloaty files, and it can't be used anyway. I will try to add a stub class soon.
  • Not all tests are currently working (they also aren't always working in the original repo).

Plenty of other changes were necessary. I recommend a thorough review & testing before merging.

@robertleeplummerjr
Copy link
Member

How are tests coming along?

@robertleeplummerjr
Copy link
Member

What kind of size savings is there, if any?

@Symbitic
Copy link
Contributor Author

34kb for gpu-browser.min.js
Surprisingly, gpu-browser.js is actually bigger (that's because Rollup doesn't remove comments unless minified - there's a plugin I can add.)

For the tests, how are they doing in this repo?
The ones that didn't pass in my repo were the ones that didn't pass when I ran a clone of this repo either.

@Symbitic
Copy link
Contributor Author

Okay, I added a plugin to remove comments.
Smaller bundles now. Only a few kilobytes but still.

While most tests pass, there are some that don't, mostly ones that involve the Array classes. Example:
argument array 1 types: single precision Array1D(3) webgl2 (1, 0, 1)
I remember those also not working in the master repo when I last ran the tests two days ago.
So far, of 2756 tests, only 62 fail.

I also added a stub HeadlessGLKernel for browser builds.

@robertleeplummerjr
Copy link
Member

I'll start looking at those next.

@robertleeplummerjr
Copy link
Member

This is still a priority, but the library has been moving along. Is there anyway you can uograde to handle conflicts?

@Symbitic
Copy link
Contributor Author

Symbitic commented Oct 29, 2019

I'm working on it. This would have been much, much, much easier if you had merged my commits first before the others. I'm basically having to redo the entire thing all over again.

Please no more commits until I finish this.

Edit 1: Okay, it looks like most test-related stuff can be merged pretty easily. How many actual feature related changes there? I think it would be easier to keep my versions and then re-add the feature-related changes once merged.

@Symbitic
Copy link
Contributor Author

FINALLY! I have FINALLY upgraded to merge all the most recent changes!

I still had to remove the functions in utils.js that used the GPU class - that's a circular dependency, and it's just not right! Nothing was using them anyway.

@robertleeplummerjr
Copy link
Member

You mention in here possibly using something other than qunit, I'm VERY open to using something else, as I want code coverage in GPU.js, and qunit doesn't easily offer it. Which of the browser AND node based testing frameworks are you familiar with that allow for easily searching and running individual tests as well as all as a whole?

@robertleeplummerjr
Copy link
Member

Fine work. I'll take it from here.

@robertleeplummerjr robertleeplummerjr merged commit 26bc244 into gpujs:develop Oct 29, 2019
@robertleeplummerjr
Copy link
Member

I pretty big issue I see so far is that we need unit tests to run in BOTH node and browser. You've changed all the unit tests to run what it seems, just in the browser.

@robertleeplummerjr
Copy link
Member

Unit tests are ran in node via npm run test test/feature for example

@robertleeplummerjr
Copy link
Member

As I suspected, if I change the path so I can test in node:

import { GPU } from './gpu';
       ^

SyntaxError: Unexpected token {
    at Module._compile (internal/modules/cjs/loader.js:723:23)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)
    at Object.<anonymous> (/Users/robert.plummer/gpu.js/test/features/add-custom-function.js:2:17)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)

@robertleeplummerjr
Copy link
Member

It looks like https://mochajs.org paired with https://www.npmjs.com/package/mochawesome may be a good alternative.

@robertleeplummerjr
Copy link
Member

I'm going to go ahead and merge some things into master, and backwards into this branch, to keep progress. Can you look into getting node unit tests running? If we need to change test runner tools, and it requires a bunch of work, we can easily build a translation tool to do it for us like this: https://gist.github.com/robertleeplummerjr/95c725d1984a1584c4db3344d1da8c39

@robertleeplummerjr
Copy link
Member

I went ahead and reverted the merge into develop for the time being. It was a force push.

@robertleeplummerjr
Copy link
Member

I opened #525 to continue the initial PR. We can continue conversation there.

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.

2 participants