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

Replace webpack with rollup #345

Merged
merged 2 commits into from
Nov 18, 2018
Merged

Replace webpack with rollup #345

merged 2 commits into from
Nov 18, 2018

Conversation

TrySound
Copy link
Contributor

This reduced the size of minified umd bundle from 56kB to 52kB.
Configurations was combined into multiconfig to simplify maintainment.

Terser is prefered over uglify as more stable package.

This reduced the size of minified umd bundle from 56kB to 52kB.
Configurations was combined into multiconfig to simplify maintainment.

Terser is prefered over uglify as more stable package.
@TrySound
Copy link
Contributor Author

/cc @stefnotch @andrevenancio

@TrySound
Copy link
Contributor Author

I think we are ready for release.

@stefnotch
Copy link
Collaborator

Looks good, I'll merge it after giving @andrevenancio some time to look at it.

@andrevenancio
Copy link
Contributor

Looks pretty good to me. Should we just update the README.md with the different import options we now have?

I assume we still need to specify something along the lines of

import { vec2 } from 'gl-matrix/ejs' ?

Or we dont @TrySound ?

@TrySound
Copy link
Contributor Author

TrySound commented Nov 18, 2018

Users may just import without and suffixes.

import { vec2 } from 'gl-matrix';

Advanced users may look at structure here and import what they need.

import * as vec2 from 'gl-matrix/vec2';

To be honest I'm not ready to work on docs because of lack of time. However this may be achieved after release. New imports may be described in release notes.

@stefnotch stefnotch merged commit 4921994 into toji:master Nov 18, 2018
@stefnotch
Copy link
Collaborator

Ok, I'm merging it then.

@stefnotch
Copy link
Collaborator

Just wanted to point out a minor observation:
When simply including dist/gl-matrix.js in your website and not importing it, you now have to include glMatrix. before everything (e.g. glMatrix.vec4.create() instead of vec4.create())

That's definitely a breaking change.

@TrySound
Copy link
Contributor Author

Hm.. I guess I skipped this part. But anyways polluting window is not good practice.

@TrySound TrySound deleted the rollup branch November 18, 2018 17:04
@stefnotch
Copy link
Collaborator

stefnotch commented Nov 18, 2018

Yeah, I'm quite fine with it. I only noticed it when answering someone's question about gl-matrix, because when testing something, I usually just run it in the browser console.

I suppose I could just run this to work around this minor inconvenience.

window.mat2 = glMatrix.mat2;
window.mat2d = glMatrix.mat2d;
window.mat3 = glMatrix.mat3;
window.mat4 = glMatrix.mat4;
window.quat = glMatrix.quat;
window.quat2 = glMatrix.quat2;
window.vec2 = glMatrix.vec2;
window.vec3 = glMatrix.vec3;
window.vec4 = glMatrix.vec4;

@TrySound
Copy link
Contributor Author

Or like this

const { mat2, mat2d, mat3, mat4, quat, quat2, vec2, vec3, vec4 } = glMatrix;

@andrevenancio
Copy link
Contributor

Yeah for es5 code I think adding to a glMatrix variable on the window makes sense and its quite a common practise. we avoid window pollution and I think most people are used to it. I suppose we might need to improve documentation with a basic example and I'm happy to pick up that myself at some point soon and do a PR. Looks great @TrySound thanks. Do you also have access to the glmatrix website @stefnotch? Is it worth adding a paragraph mentioning all the module options for more advanced users?

@TrySound
Copy link
Contributor Author

@stefnotch Could you also publish this prerelease on npm? You may use npm publish --tag=next and the version like 3.0.0-0

@stefnotch
Copy link
Collaborator

Do you also have access to the glmatrix website @stefnotch? Is it worth adding a paragraph mentioning all the module options for more advanced users?

Yes, I do. (It's just GitHub pages)

@stefnotch
Copy link
Collaborator

@TrySound Regarding publishing it to npm:

npm ERR! This package has been marked as private
npm ERR! Remove the 'private' field from the package.json to publish it.

Do I have to publish every single sub-package or...?

@TrySound
Copy link
Contributor Author

I mentioned in #343
image

@stefnotch
Copy link
Collaborator

Oh, thank you. I'm sorry for not properly reading it the first time.

I published it to npm now

@TrySound
Copy link
Contributor Author

Great! Thank you

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.

4 participants