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

Bundle jquery.uls and use language-data as dependency #394

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

santhoshtr
Copy link
Member

Changes

Introduce rollup based bundler to create single js, css files.
This allows easy usage and no need to include every source code files
explicitly. Fixes issue #326

Add language-data as dependency to the library
Instead of using scripts to clone the language-data repo and then create
our javascript files from it, directly use language-data as dev-dependency.
The final bundle will include language-data. So it is not an external
dependency for bundle.

Approach

To avoid any breakage in consumers, no change in jquery plugins were
introduced. No method signature change too. All plugin definitions
moved to src/index.js. index.js also import the language-data
and alias that to $.uls.data.

So, jquery.uls.data.js and jquery.uls.data.utils.js files were deleted.
The scripts to generate this files was also deleted.

All other changes in jquery.uls.*.js is just indendation change
because of the removal of IIFE. The resuling bundle will have IIFE.
IIFE block has to be removed to expose the classes for exporting
as modules.

Examples were updated to refer javascript, css and styles from dist
folder.

Future steps

Introduction of build step allows

  • To modernize the codebase to new versions of javascript
  • To drop jquery dependency and optionally keep the jquery plugins
  • Remove grunt based CI system

@amire80
Copy link
Contributor

amire80 commented May 30, 2021

This is probably good, but:

  1. I need to go over all the utils functions one by one and verify they are all fully migrated to the ones in language-data and fully covered by tests. I have a vague recollection that there were some gaps, although I might be wrong.
  2. I'm not familiar enough with the relevant technologies like rollup and the little details of package.json to make a decision about merging it. I'd love to learn, though ;)
  3. It's not clear to me how will the updating of language-data work. Will someone need to regularly run npm install in jquery.uls, or will it be updated automatically somehow? And how about the ULS MediaWiki extension? It should probably be documented in README.

@amire80
Copy link
Contributor

amire80 commented May 30, 2021

OK, I think I've found some issues.

Most notably, in language-data there are no JS tests for these util functions:

  • getLanguagesByScriptGroup
  • getLanguagesByScriptGroupInRegion
  • getLanguagesByScriptGroupInRegions

Their functionality in jquery.uls and in language-data appears to be different, too.

@santhoshtr
Copy link
Member Author

Their functionality in jquery.uls and in language-data appears to be different, too.

Can you please file a bug report with details in language-data repository about this? I did a quick comparison of code for these three methods and I could not find difference.

Most notably, in language-data there are no JS tests for these util functions:

The tests were just copied from jquery.uls when language-data library was created. We need to add test wherever missing. But that is general improvements for language-data library. Not directly related to this pull request.

1. I need to go over all the utils functions one by one and verify they are all fully migrated to the ones in language-data and fully covered by tests. I have a vague recollection that there were some gaps, although I might be wrong.

This need be addressed, But I think this is independent of what is tried in this pull request - at least technically. Any upstream changes released in language-data comes to jquery.uls as the result of this pull request.

3. It's not clear to me how will the updating of language-data work. Will someone need to regularly run `npm install` in jquery.uls, or will it be updated automatically somehow? And how about the ULS MediaWiki extension? It should probably be documented in README.

This is done as dependency updates. We have defined a dependency like this:
"@wikimedia/language-data": "^1.0.2",

Whenever we release new version of language-data, in this repo, we need to change this line in package.json and use the latest version number. Then, along with any other jquery.uls changes, we can run npm run build, npm publish etc to release new version of jquery.uls

package.json Outdated Show resolved Hide resolved
examples/limitedLanguageList.html Outdated Show resolved Hide resolved
<script src="../src/jquery.uls.lcd.js"></script>
<script src="../src/jquery.uls.languagefilter.js"></script>
<script src="../src/jquery.uls.core.js"></script>
<script src="../jquery.uls.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

Running the examples directly from the root examples directory no longer works. We should update the README.md file to mention that the users should build the files, and then run the examples from the dist/examples folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Once this PR is done, I plan to create a static application running from dist folder in github pages or netlify or similar places. Along with that I will address this confusion

santhoshtr and others added 4 commits July 17, 2021 15:25
Changes
=======

Introduce rollup based bundler to create single js, css files.
This allows easy usage and no need to include every source code files
explicitly. Fixes issue #326

Add language-data as dependency to the library
Instead of using scripts to clone the language-data repo and then create
our javascript files from it, directly use language-data as dev-dependency.
The final bundle will include language-data. So it is not an external
dependency for bundle.

Approach
========

To avoid any breakage in consumers, no change in jquery plugins were
introduced. No method signature change too. All plugin definitions
moved to src/index.js. index.js also import the language-data
and alias that to $.uls.data.

So, jquery.uls.data.js and jquery.uls.data.utils.js files were deleted.
The scripts to generate this files was also deleted.

All other changes in jquery.uls.*.js is just indendation change
because of the removal of IIFE. The resuling bundle will have IIFE.
IIFE block has to be removed to expose the classes for exporting
as modules.

Examples were updated to refer javascript, css and styles from dist
folder.

Future steps
============

Introduction of build step allows

* To modernize the codebase to new versions of javascript
* To drop jquery dependency and optionally keep the jquery plugins
* Remove grunt based CI system
@Abijeet Abijeet self-requested a review July 17, 2021 10:54
Copy link
Member

@Abijeet Abijeet left a comment

Choose a reason for hiding this comment

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

Changes look good, approved.

There were some conflicts which when resolving overrode some changes, I've checked and added them back.

@Nikerabbit
Copy link
Member

I do like this approach, but it is a breaking change:

These are solvable issues, but I think this warrants a major version bump, and I'd like us to also reach out to the projects using jquery.uls for their feedback and concerns, and we probably should prepare patches for ULS/Translate before we merge it. Else we may end up in a situation we cannot update jquery.uls until those patches are ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants