jquery.uls icon indicating copy to clipboard operation
jquery.uls copied to clipboard

Bundle jquery.uls and use language-data as dependency

Open santhoshtr opened this issue 4 years ago • 4 comments

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

santhoshtr avatar May 14 '21 12:05 santhoshtr

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 avatar May 30 '21 07:05 amire80

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.

amire80 avatar May 30 '21 14:05 amire80

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

santhoshtr avatar Jun 02 '21 09:06 santhoshtr

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

  • Translate extension depends on jquery.uls.grid (as exposed by ULS) -> could be update to jquery.uls
  • Bunch of extensions reference jquery.uls.data separately -> could be updated to jquery.uls - but loading of data is often delayed because it is a big module, so there is a performance concern
  • Other repos using jquery.uls also need updating: https://codesearch.wmcloud.org/search/?q=jquery%5C.uls&i=nope&files=&excludeFiles=&repos=

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.

Nikerabbit avatar Jul 19 '21 13:07 Nikerabbit