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

refactor: Replace uptown with ES2015 classes #209

Merged
merged 2 commits into from
Jan 24, 2019
Merged

refactor: Replace uptown with ES2015 classes #209

merged 2 commits into from
Jan 24, 2019

Conversation

realityking
Copy link
Contributor

Breaking change: Inheriting from a minim type now requires using native JS (class extends or prototypes) instead of uptown methods.

I'd like to propose removing the uptown dependency from minim and using ES2015 classes instead. This is a breaking change due to different semantics of ES2015 and uptown but it reduced code size & complexity and makes it easier for IDEs to understand the code (for autocomplete)

Perhaps the biggest downside is that users who want to use minim on the frontend will have to use Babel if they care about IE11 comparability. IMHO that's a fair trade off but others might feel otherwise.

@smizell
Copy link
Contributor

smizell commented Jan 15, 2019

Please feel free to ignore me as I'm just adding a thought here :)

This is super cool. I think if you wanted to keep support for IE11 and non-Babel builds, uptown could be used to mutate all of the classes in a given namespace (I think). There is an extendify function that can take a class and add the .extend class method. I'm not sure if you all need this kind of support, but it could be a way to do this and keep older support. You could probably write a little function that takes a minim namespace and calls .extendify on each class.

https://github.com/smizell/uptown/blob/2fc1af0d3b7736fdc99fc980b34469f0ee598d31/lib/uptown.js#L38

Just a thought!

@char0n
Copy link
Contributor

char0n commented Jan 16, 2019

@realityking If you run the code through the babel7, than you can get ES5 code that runs on every environment. And what get's distributed via npm will the babelified ES5 instead of original source code.

@realityking
Copy link
Contributor Author

@char0n Definitely an option. I saw that babel was removed from several Apiary packages so I wasn't sure if that was something that's wanted. Should I make that change? And what browsers should be supported?

@honzajavorek
Copy link
Contributor

I removed Babel from Dredd and Dredd Transactions, as they run solely on Node.js at this moment. There have been thoughts about making them browser-compatible, but that's long-term idea with a lot of prerequisites. Until that happens, Babel would be only an obstacle to the development.

@realityking
Copy link
Contributor Author

Alright. I'll make a PR to add babel to minim. Any comment on desired browser support?

@pksunkara
Copy link
Contributor

"browserslist": [
     ">0.25%",
     "not op_mini all",
     "ie 11"
 ]

This allows using modern Js features while maintaining compatability with older browsers.
@realityking
Copy link
Contributor Author

realityking commented Jan 23, 2019

Added babel to the PR 🙂

The browserified build more than 30KB smaller than before.

Copy link
Contributor

@pksunkara pksunkara left a comment

Choose a reason for hiding this comment

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

Just a few comments.

test/subclass-test.js Outdated Show resolved Hide resolved
.babelrc Show resolved Hide resolved
Breaking change: Inheriting from a minim type now requires using native JS (class extends or prototypes) instead of uptown methods.
@pksunkara pksunkara merged commit 07c9435 into refractproject:master Jan 24, 2019
@realityking realityking deleted the uptown branch January 24, 2019 14:02
kylef added a commit that referenced this pull request Feb 22, 2019
#209 removed some
documentation coverage which causes other classes that reference the
documentation in other libraries to fail.
kylef added a commit that referenced this pull request Feb 22, 2019
#209 removed some
documentation coverage which causes other classes that reference the
documentation in other libraries to fail.
kylef added a commit that referenced this pull request Feb 25, 2019
#209 removed some
documentation coverage which causes other classes that reference the
documentation in other libraries to fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants