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

fix: Use rollup and fix define module issues #434

Merged
merged 5 commits into from
Aug 16, 2022

Conversation

timgl
Copy link
Collaborator

@timgl timgl commented Aug 15, 2022

Changes

In progress. Only thing missing is minification of dist/array.js which I couldn't figure out with rollup

Checklist

@timgl timgl requested a review from mariusandra August 15, 2022 15:53
@mariusandra
Copy link
Collaborator

I fixed what was broken and changed the export type to iife, short for Immediately Invoked Function Execution. This made everything simpler. All the array code code is now executed in a enclosed function/scope, letting it handle injecting itself onto window, as it's doing anyway.

There's no possible conflict with a global define anymore, as everything is locally scoped.

We went down some bytes in size as well! 114k -> 79k, a 30% reduction!

# before
-rw-r--r--  1 marius  staff   114K Aug 16 10:55 array.js
-rw-r--r--  1 marius  staff   421K Aug 16 10:55 array.js.map

# after
-rw-r--r--  1 marius  staff    79K Aug 16 10:49 array.js
-rw-r--r--  1 marius  staff   409K Aug 16 10:49 array.js.map

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

All looks sensible to me!

@timgl
Copy link
Collaborator Author

timgl commented Aug 16, 2022

Nice! Do you feel confident releasing this as is, or should we do some testing somehow?

@mariusandra mariusandra added the bump minor Bump minor version when this PR gets merged label Aug 16, 2022
@mariusandra
Copy link
Collaborator

Just to be sure, I built, published and tested the new array.js locally on kea-docs, and everything worked. Events came in, sessions got recorded. The github test with IE11 even passes. Thus yep, let's release and get it on cloud for further testing.

@mariusandra mariusandra merged commit b811fac into master Aug 16, 2022
@mariusandra mariusandra deleted the use-rollup-fix-define branch August 16, 2022 11:46
@pauldambra pauldambra mentioned this pull request May 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

array.js uses a "define" that conflicts w/ (Adobe Commerce) Magento's requirejs
3 participants