Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Improve the UX/DX for creating a module for framework - Closes #2863 #2865

Merged
merged 9 commits into from
Feb 15, 2019

Conversation

nazarhussain
Copy link
Contributor

What was the problem?

Currently we are expecting any module to export a json object in following structure, which was not aligned with other framework structs.

How did I fix it?

Introduced a new BaseModule class.

Review checklist

@MaciejBaj
Copy link
Contributor

Please resolve the conflicts.

Copy link
Contributor

@yatki yatki left a comment

Choose a reason for hiding this comment

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

In general, I'm kind of against having a BaseModule class at this moment. This change just makes the code more dependent on ES6 Classes while we can simply validate the JSON Schema for the module with schema validators.

Maybe this topic may require an STM, but I'd prefer to add complexity to the code when it's actually needed.

},

/**
* Supported configurations for the module with default values
*/

defaults: {},
get defaults() { return {}; },
Copy link
Contributor

@yatki yatki Feb 13, 2019

Choose a reason for hiding this comment

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

I would avoid getters and setters in javascript. Because,

  • I think it makes code a bit hard to read,
  • It's not widely adopted by js developers,
  • Stubbing or spying is still possible (https://stackoverflow.com/a/43744255) but it's not as straightforward as faking class methods.
  • We get no feedback or exception if we make a typo like obj.defults = 'something'.

So I would prefer to have get and set methods instead of getters and setters.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if we only have getter it will throw an error if we do * obj.defults = 'something'
but it is very confusing to use defaults with getter because in babel/es rule you can export default. so,

export default class X {
  default: 'x',
}

and you can do
import X from './above.js'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this project unless we have babel or typescript we can not use export default. Event though its not confusing to if we have const options = module.defaults and export default. Both have totally different reasons and syntax.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yatki getters are used if you don't want to a setter ;)

Yes you can ask to freeze an object that will not be modified, but then it can not be modified by internal logic as well. So getters are a valid use case and in my experience its widely adopted language struct overall, for JS its newly announced language feature so will take some time for users to adopt.

framework/src/controller/application.js Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/errors.js Outdated Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
framework/src/controller/controller.js Outdated Show resolved Hide resolved
lsilvs
lsilvs previously approved these changes Feb 15, 2019
@MaciejBaj MaciejBaj merged commit d866dfe into development Feb 15, 2019
@MaciejBaj MaciejBaj deleted the 2863-base-module branch February 15, 2019 17:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the UX/DX for creating a module for framework
5 participants