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

Migration stage #1180

Merged
merged 2 commits into from Dec 3, 2018

Conversation

Projects
None yet
4 participants
@DylanPiercey
Copy link
Contributor

commented Dec 3, 2018

Description

Adds a migration stage to the compiler which is exposed through the taglib api via a new migrate option on both tags and taglibs.

This PR also moves over some of the existing transform based migrations (assign, var, invoke, and imperative rendering). It also adds a new migration for (and deprecates) inline control flow directives (<div if(x)>`).

This piggybacks off of #1179.

Checklist:

  • I have read the CONTRIBUTING document and have signed (or will sign) the CLA.
  • I have updated/added documentation affected by my changes.
  • I have added tests to cover my changes.

@DylanPiercey DylanPiercey force-pushed the migration-stage branch from 9a05620 to ab49759 Dec 3, 2018

@codecov

This comment has been minimized.

Copy link

commented Dec 3, 2018

Codecov Report

Merging #1180 into master will decrease coverage by <.01%.
The diff coverage is 94.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1180      +/-   ##
==========================================
- Coverage   90.48%   90.48%   -0.01%     
==========================================
  Files         314      317       +3     
  Lines       11982    12042      +60     
==========================================
+ Hits        10842    10896      +54     
- Misses       1140     1146       +6
Impacted Files Coverage Δ
src/taglibs/core/core-transformer.js 84.84% <ø> (-1.76%) ⬇️
src/compiler/CompileContext.js 92.72% <ø> (-0.04%) ⬇️
src/compiler/taglib-loader/Taglib.js 79.62% <100%> (+1.62%) ⬆️
src/compiler/taglib-loader/Tag.js 85.14% <100%> (+0.61%) ⬆️
src/compiler/Compiler.js 95.94% <100%> (ø) ⬆️
src/compiler/Parser.js 97.31% <100%> (+0.1%) ⬆️
src/taglibs/migrate/var-tag.js 89.28% <100%> (ø)
src/taglibs/migrate/control-flow-directives.js 100% <100%> (ø)
src/compiler/taglib-loader/loadTagFromProps.js 89.08% <100%> (+0.14%) ⬆️
src/taglibs/migrate/util/printJS.js 100% <100%> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e68893...6c37027. Read the comment docs.

@coveralls

This comment has been minimized.

Copy link

commented Dec 3, 2018

Coverage Status

Coverage decreased (-0.002%) to 90.483% when pulling 6c37027 on migration-stage into 3e68893 on master.

@DylanPiercey DylanPiercey force-pushed the migration-stage branch from ab49759 to 6c37027 Dec 3, 2018

@mlrawlings mlrawlings merged commit 7d37fe3 into master Dec 3, 2018

5 checks passed

codecov/patch 94.54% of diff hit (target 90.48%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +4.05% compared to 3e68893
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@ianvonholt

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

Was there any particular reason to remove inline control statements?

<div if(x)>Blargh</div>

is a bit more streamline than

<if(x)>
  <div>Blargh</div>
</if>
@DylanPiercey

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2018

@ianvonholt it was something we have been talking about for a while, although I wish we were a bit more transparent about it.

Here is the reason:

We have seen this particular syntax is frequently used in ways that have caused confusion among developers. Specifically it's very easy to glance over where these (important) control flow statements are taking place. I think our example illustrates one of the reasons behind this, which is that it can simply be very tricky to keep on top of formatting this in a readable way especially when attributes start to pile up.

The value add for this feature was primarily around terseness and readability (which is not something we want to move away from) however I think In this case it ended up becoming more of a foot gun in the language. In examples (like the one you showed) it does look nice, but when there are other attributes on the tag (as is often the case in real-life, non-example code) readability actually suffers.

On top of this, these directives are yet another thing that new developers have to learn when on boarding to the language and also a feature that makes the framework a bit harder to maintain (two ways to do everything can complicate the code base).

Because of this, and because it is an extremely easy feature to automatically migrate, we've decided deprecate it here in Marko 4. It will be removed in Marko 5.

Our recent work has been focused on creating a migration layer in the Marko compiler so that most of these features you are seeing deprecation warnings for can be automatically migrated by marko migrate.

@mlrawlings

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

As @DylanPiercey mentioned, this is something we've been discussing for a while. It happened here because not doing so would have required some extra effort in implementing some of the things in this PR (and future migration related PRs).

One of the reasons we do deprecations in advance is so that users can see these warnings and take things up with us if we're missing a use case. So this isn't 100% set in stone, but I do think it's the right path forward.

@DylanPiercey DylanPiercey deleted the migration-stage branch Dec 6, 2018

@ianvonholt

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

@mlrawlings @DylanPiercey Thank you for the input, I do get the reasons for deprecation when there are elements that have a lot of attributes.

Speaking of Marko v5, is there a roadmap of features that are being considered?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.