Skip to content

Modularize itoolkit.js #20

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

Closed
abmusse opened this issue Jan 24, 2019 · 5 comments
Closed

Modularize itoolkit.js #20

abmusse opened this issue Jan 24, 2019 · 5 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@abmusse
Copy link
Member

abmusse commented Jan 24, 2019

I propose we modularize the classes and methods within itoolkit.js

Move The following to there own files:

  • iConn class move to iconn.js
  • iPgm class mov to ipgm.js
  • iSql class move to isql.js
  • iCmd function move to icmd.js
  • iSh function move to ish.js
  • xmlToJson move to utils.js

For xmlToJson & __getClass functions

I propose we create a new file called utils.js and move both there since both are commonly used across the classes in the project.

Add exports to still allow access to all relevant Classes/Methods through itoolkit.js

In the end itoolkit.js will simply be our main entry point

module.exports = {
  iConn: require('./iconn').iConn,
  iSh: require('./ish').iSh,
  iQsh: require('./iqsh').iQsh,
  iPgm: require('./ipgm').iPgm,
  iSql: require('./isql').iSql,
  iWork: require('./iwork').iWork,
  iProd: require('./iprod').iProd,
  iUserSpace: require('./iuserSpace').iUserSpace,
  iNetwork: require('./inetwork').iNetwork,
  iObj: require('./iobj').iObj,
  iDataQueue: require('./idataq').iDataQueue,
  xmlToJson: require('./utils').xmlToJson
};

@dmabupt @kadler @ThePrez @markdirish

What do you all think?

If approved I will submit a PR with these changes

@markdirish
Copy link
Contributor

I just looked at the file and with all the classes it looks like a good candidate for modularization. Just as a personal preference, when exporting a class from a file that only contains that class, I'm a fan of:

module.exports = class iConn {
...
}

And then in the itoolkit.js (or wherever) you only need const iConn = require(iConn);, or as a property in this case. Also maybe call Object.freeze on module.exports just as a best practice so others can't change the properties.

I think your proposed changes will be good to start refactoring itoolkit.js, looks like it needs a little TLC.

@abmusse abmusse added enhancement New feature or request proposal labels Jan 24, 2019
@kadler
Copy link
Member

kadler commented Jan 24, 2019

SGTM.

@kadler
Copy link
Member

kadler commented Jan 24, 2019

I just looked at the file and with all the classes it looks like a good candidate for modularization. Just as a personal preference, when exporting a class from a file that only contains that class, I'm a fan of:

module.exports = class iConn {
...
}

And then in the itoolkit.js (or wherever) you only need const iConn = require(iConn);

This seems fine to me, so long as we don't expect the exports to ever change or the user is never expected to import iConn directly - just through itoolkit.

@abmusse
Copy link
Member Author

abmusse commented Jan 28, 2019

Open PR #28 for this issue

@IBM/ibmi-open-source

Feel free to review and comment

markdirish added a commit that referenced this issue Jan 28, 2019
Modularize itoolkit.js see issue #20
@markdirish markdirish mentioned this issue Jan 28, 2019
9 tasks
@abmusse
Copy link
Member Author

abmusse commented Jan 30, 2019

PR merged in branch v1.0-dev closing this issue

@abmusse abmusse closed this as completed Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants