Upgrade Zola Ember & Set Node Version #1058
Comments
|
I totally hear you on upgrading labs-ui being a headache but I really do not want us pulling stuff out of shared repositories. The Labs portfolio doesn't do the best job of "sharing code" (be it through npm packages, RESTful apis, or other means) as it stands today, so I'd rather not add to that by pulling out and duplicating the shared code we do have. You're on the right track with the sass stuff but maybe I can clear things up - in this context, "SASS" refers to a language-agnostic "spec" that has been implemented in several languages. As you pointed out, node-sass is a thin wrapper around the C implementation. "Dart Sass" is an implementation written in Dart that has been the "default" one for a little while now - so it gets new features the soonest and generally has the best support. It's probably worth looking into if we can move to this package which is the JS distribution of Dart Sass (as opposed to the one we currently use which comes from another npm package named |
|
Fwiw, it looks like our node-sass dependency comes from |
Yes this is my understanding.
That's what I'm trying first.
I'll try that first.
That's promising. |
|
@TylerMatteo so ideally we will end up with this: Labs-UI shared code will introduce correct Sass deps (Dart-based The reason I'm exploring other paths is that the main goal as far as I understand is to upgrade things to Ember 3.19. However, I'm seeing a number of hurdles introduced by these sass dependencies and looking to simplify in hopes of successfully completing the upgrade. |
|
Yeah you're correct about the upgrade being the main objective, I just want us to at least make a serious effort to do so without introducing other forms of tech debt into the portfolio. That being said, I'm not necessarily expecting you to reintegrate dependencies where they've already been split out, such as ZAP search - my hope is that would be relatively easy for the team to handle once all the upgrades are complete. |
|
Totally separate, but wow, layers-api:
I don't even know if that application is set up to support compression... |
|
Okay, pulling down labs-ui to develop locally and hopefully solving some of this stuff |
|
Promising: swapping in the new dart-baesd ember-cli-sass drops node-gyp and node-sass as expected:
I've opened a PR for the upgrade of labs-ui, but I'm going to test it locally as well to see how a version bump of it might affect consumer apps: NYCPlanning/labs-ui#78.
|
|
Stepping back and running dependency lint here's where we are:
I have a branch for another missing dependency that might've popped up from some dependency drift or node versioning (who can say): mg/escape-dependency-hell Fixed dependency lint issue by upgrading fort-awesome... Am I being trolled? It's crazy, it almost seems to resolve stuff non-deterministically (regenerating yarn.lock for example). No matter what it seems to resolve @fortawesome/ember-fontawesome with different versions. Fun! Going to punt on that path for now... Okay one source of conflicting versions: labs-ui and labs-ember-search both specific ember-power-select as dependencies. Depending on the version, labs-ui asks for power-select 4. ember-search is still on version 2. Hence, upgrades lead to build time errors... turning towards upgrading ember-search now... |
|
All sorts of strange behavior on labs-ember-search, and that one has a small number of dependencies. I'm going to keep chipping away that one and see what I learn... The (ember-cli-search) package's current ember-cli version requires node 10. Upgrading ember-cli griefs me so far... Made some progress:
In order to upgrade ember-power-select, I will need to upgrade ember-source due to the ember-basic-dropdown dependency... Update: got ember upgraded and also ember-power-select! |
|
Thanks for documenting all these trials and tribulations 😆 The dependency path to ember-basic-dropdown definitely rings some bells to the last time I tried to untangle this. Sounds like you're making some progress? |
|
As you're digging into labs-ember-search, just want to point you at this PR I did back in December. I had to revert some upgrades so that I could release a version that was still compatible with older apps but included some data-related updates we needed. Not sure if it's relevant to your work here, but I'd imagine you might end up re-implementing some of the changes I reverted in this PR so just wanted to make sure you're aware. It looks like the changes I reverted got it all the way up to |
|
@TylerMatteo okay, thank you for that context! I'm trying to avoid upgrading things too far. My goal is to test the two main dependencies in consuming apps before committing to them. I've upgraded to 3.11. All that to say... I can see how all this dependency mess has kept everyone and everything "stuck" in a really bad way! Going to keep digging... |
|
Let me know if you prefer I not document things here, just need to keep track of where I left things in case I need to switch context to something else. Currently: using yalc to test labs-ember-search with other consuming apps and noting common pitfalls... |
|
There's a 3rd dependency to worry about, too: ember-mapbox-composer. Getting recurring issues with fortawesome, seeing this: Build Error (broccoli-persistent-filter:Babel > [Babel: @fortawesome/ember-fontawesome]) in @fortawesome/ember-fontawesome/components/fa-icon.js
[BABEL] /Users/mattgardner/labs-zola/@fortawesome/ember-fontawesome/components/fa-icon.js: api.assumption is not a function (While processing: "/Users/mattgardner/labs-zola/node_modules/labs-ui/node_modules/@babel/plugin-transform-modules-amd/lib/index.js")"Fix" is to Current "working" branch I'm using is mg/escape-dependency-hell-simplify... Separately: NYCPlanning/labs-ember-search#44 is a PR where I do gentle upgrades, only a few minor versions. Next step I need to clean it up by moving some things back into the "dependencies." For ember addons, the dependencies hash tells Ember which explicit dependencies it needs (as opposed to devDependencies). |
|
Going from labs-ui 0.0.25 to 1+ is going to be pretty bad because labs-zola actually directly extends some of the components in there. Two paths:
|
|
Path 1 leads to some blockers that are usually solved by codemods, however, there is some behavior going on in the linked code below that I think will probably totally block a full upgrade to Ember 3.19: I'll see where I can get with this. I at least have a solution path towards 3.11. Last ended on: It's necessary for soem reason to includer @glimmer/component as a dependency on consuming apps (something about font awesome). However, I can only get 0.0.14 (alpha.13) to work. No idea way. Going from labs-ui 0.0.25 to 1 breaks a bunch of functionality I think because it switches from classic component sytnax to native class syntax. Might be related to how glimmer deals with this at the addon level... |
I think documenting all this stream-of-consciousness stuff here is great. Once you have any cleaned up PRs that are ready to review, you can describe what you actually ended up doing in those (within reason, don't need a book for each PR). I'm totally onboard with you doing multi-step upgrades, such as going to 3.11 before continuing on to 3.19 if you think that will be helpful. I'd imagine it might help break up the work. The extending thing in Zola is a little concerning. Do you think that is an artifact of a previous attempt to move some of that labs-ui code out of the library and duplicate it into Zola directly? |
Great!
I think it's just an artifact of bad design... I think I wanted to use some composability with the existing labs-ui components (I wanted programmatic access to rendered children of "layer-group-containers", for example, "Zoning and Land Use": but I think the problems are fixable. I'm going to try to keep things away from "refactor" zone and firmly inside of "codemod/syntax updates". |
|
Note to self: assess ZoLa deprecation warnings if we get to 3.19. |
|
Possible helpful tool: https://github.com/raineorshine/npm-check-updates#doctor-mode |
|
When you feel like you have your thoughts straight (which I know is hard to do with this stuff), can you give me a brief summary of where you're at with all of this? In particular, I'm curious which labs-built dependency you think need upgraded before we can upgrade Zola (and, presumably, the other front ends). It sounds like at least labs-ember-search, labs-ui, and ember-mapbox-composer would be on that list? Any others? |
|
I'm still in the midst of discovery mode so I won't have a concise answer for you on that just yet but give me some more time and I'll see where I've landed. There are a lot of moving parts (this is less straightforward because of the requirement to keep labs-built addons/dependencies rather than pulling them into consumer apps). |
|
Just curious, have you tried pointing the apps at version 1.1.0 of labs-ui after upgrading them? It looks like Andy got it upgraded to ember cli 3.17 at one point (though that might not be helpful if you're trying to upgade to 3.11 first before continuing on to 3.19). Feel free to ignore me here if you were aware of this version but don't think it's helpful, just throwing it out there because I just came across it myself. |
|
@TylerMatteo yeah, I tried that, the problem is that things were switched to pure glimmer components, which is a breaking change (hence the major semver). And the handlebars syntax for some of the extended components do not work with pure glimmer components. there's some deprecation stuff (like referencing actions with strings) that introducing glimmer components breaks... I don't know if that makes sense but basically the labs-ui 1+ version prematurely introduces a breaking change (glimmer) that ZoLa (after extending it) needs refactoring to support... Separately, I thought of a different strategy. So, the thing I want to do is reduce the number of variables WHILE upgrading... because the labs addons are sometimes opionated about which dependencies are required, sometimes those are inappropriately specified in "dependencies," I am going to pull in (duplicate) the addons temporarily while upgrading ZoLa. That way I can focus on which depencies I know for sure that I'm dealing with, and not needing to deal with Anyway this is my current thinking. |
Gotcha. Is it correct to say that labs-ui could be upgraded to ember 3.19 without switching to pure glimmer components? In other words, did Andy switch to glimmer components so that he could upgrade or was it just taking advantage of a new feature that the upgrade enabled?
Seems reasonable enough to me. Maybe we can squash commits with the duplicated components before we go to master, just to keep the commit history less confusing. |
|
Note that I cannot get Github Actions to run the tests after upgrading to node 12... 🤷 One theory: when I removed Github-hosted packages just to test the Github-hosted packages hypothesis, it caused the tests to fail right out of the gate (https://github.com/NYCPlanning/labs-zola/runs/7229494002?check_suite_focus=true). Re-adding them (NPM-hosted), it still stalls... but I think it's because it's stalling from some WebGL-related reason (the app won't continue loading unless WebGL is available). It's probably just sitting on the first test, waiting for it to finish. |
|
FYI @TylerMatteo I'm pretty stuck on getting this test suite to run in CI. I'll keep poking around. These tests run and pass locally, naturally. But any tips for debugging CI would be appreciated! |
|
Hey, I just submitted a review on your PR, sorry for the delay. I'm psyched to see it actually goes all the way to 3.19! I'm afraid I probably won't be much help when it comes to debugging tests but I'm happy to look into it next week. |
|
Just fyi - once we have your PR approved, I'd like to hold off on merging into develop until we get a small change in related to another project. It should be in early next week. For now, just plan on waiting for me to click the merge button on your PR, even its approved. |
Great! Sorry all the weird console logs and commenting out things were attempts at debugging GH Actions. My tests pass locally but since upgrading Node, they simple stall out in CI. I believe this has something to do with webgl not loading in the headless environment, and the "ready" state never being achieved (hence the timeouts). Once I actually get tests to pass in CI (again, they pass locally 😄), I will remove those junk lines. Thanks again for the review! I hope to figure out this GH actions stuff asap. Then on to applicant portal backend... LearningsSo far, all I can glean from this is that some combination of Fastboot, Ember Mapbox Composer, Main Map, and the Bookmarks feature is what's causing tests to stall... Without the map, it seems to stop at the 3rd bookmark test... with the map, not sure yet... it seems to never get beyond the first test. Going to see if upgrading ember-qunit will help with the asynchrony isseus: https://github.com/emberjs/ember-qunit/blob/master/docs/migration.md |
|
Hey Matt, sorry I've been MIA. How's it going with trouble shooting those tests? I saw your email and am happy to schedule something for early next week if you still want to chat. |
|
Hi @TylerMatteo no problem. I'm fairly open on Monday if you want to hop on a call then. |
|
After a brief call, we settled on:
|
|
In this branch, there are two commit-hash-based dependencies. One is for mapbox-composer. I made a PR here that needs a review. The other is for ember-needs-async which looks to be abandoned. I'll try to PR there but we may not get a new release and might need to fork it to the organization. |
|
@allthesignals Just took another look at your PR and resolved a bunch of conversations. This is looking pretty close to being ready for a merge into develop to me. The one thing I'd like for us to do is have those commit hash dependencies be pointing to commits in repos owned under the NYCPlanning Github org. I'm out of the office next week but when I'm back we can try to get your ember-mapbox-composer PR (please take a look at my comment on that PR) merged in and publish a version so that we don't need the hash at all. As for ember-needs-async, I can set up a fork of under our GH org when I get back. Almost there! Hopefully you've been able to make some progress on applicant portal backend and aren't totally blocked by your Zola PR languishing a little longer. |
|
Thanks @TylerMatteo — sounds good. FYI I have an applicant protal backend PR up. |
|
Okay, two commit hash deps:
Awaiting resolution for this
Will point to org fork when you're back |
|
Ok I forked ember-needs-async and gave you access so you should be able to push your branch directly if you'd rather do that than PRing from your own fork, up to you. Once we have that merged, you can update the hash here to the one under the NYCPlanning fork I left a comment on your composer PR as well. Just need to tweak |
|
@allthesignals Please see my previous comment. Once you update your composer PR with those changes and open a PR against our new ember-needs-async fork, I can see about getting these changes out the door. |
|
Thanks @TylerMatteo, been out of on holiday the past week. Will take a look ASAP. |
|
Hey @allthesignals, how difficult do you think it would be to remove the dependency on |
|
I think so... all ember-needs-async does is allow usage of fetch (requests to the store) inside templates. Using ember-concurrency might help there.. or just copying the ember-needs-async code into the repository and updating dependencies accordingly. |
|
@TylerMatteo what are the things I owe your? Responding to feedback on the PR? |
|
@TylerMatteo alright, this should be good to go... I ran tests locally and got passing:
CI test suite still doesn't work, of course... but that should be discussed in #1093 ... |
|
this GH actions test runner issue is becoming sisyphean to me but i can't stop poking around at it... attemting minimal repro here: https://github.com/allthesignals/labs-zola-gh-actions-failure-reproduction |
|
@allthesignals I see that you've updated the mapbox-composer dependency to 0.3.0 and updated the hash for needs-async, so those issues look good to me. If you would have to copy over all of that code to remove ember-needs-async, we may as well just keep using that fork. I was hoping there might be a way to rebuild that one component where we use it to have a one-off usage of ember-concurrency directly, but I don't want to keep moving the goal posts here. A couple things:
cc PR#1059 |
Yes
All of them
Please use the PR to request changes so that we can stay organized. |
|
@TylerMatteo let's document other adjacent observations in separate tickets (the needs-async thing, etc) |
|
@TylerMatteo the blocker to moving up to Node 14 is ember-mapbox-composer requires ^12. I think you changed it to require this level of node. |
|
@TylerMatteo I pointed ember-mapbox-composer to a commit of mine. It needs to permit node 14 for it to work here too. The other tasks are complete. Note that testing in headless chrome does not yet work. I added a comment in the README but testing needs to happen by visiting /tests. |
|
Node 14/Ember 3.19 upgrade is live 🥳 |
This issue is closed when Ember is version 3.19 and the app builds in Netlify for Node 14/16
Development issues
"yarn add node-sass never finish"
See workaround: yarnpkg/yarn#5129 (comment)
TODO: should switch out labs-ui, rework the sass-based styling dependency, and consider postcss to compile sass. Or, consider a static build of labs-ui styles... then do some overrides where necessary...
Cannot find module '@babel/plugin-proposal-private-property-in-object'
Simply
yarn addthis dependencyFont Awesome issues
Build Error (fontawesome-svg-core): acorn.Parser.extend is not a function
Currently unknown....
General Direction so far
A huge issue and barrier here is node-sass and foundation. Foundation-Sites has some known SemVer "violations" which make upgrading difficult and maintenance unsustainable.
Further, node-sass is a C dependency, which is high overhead for setting u pthe project.
I dealt with this in ZAP Search frontend by pulling in the compiled "Labs UI" stylesheets as a single asset and building on top of that. Pre-processing was moved away from node-sass (deprecated) and towards PostCSS.
Because node-sass is deprecated (I believe it's now "Dart Sass" or something), we should move away from this and towards a high-level compiler (node-based). This will elminate the need for a C compiler for SASS (which is wild). I think for some orgs with extremely complicated stylesheets, a fast compiler is very nice, but that is not our situation currently.
What this means is needing to move some things away from the labs-ui shared addon. I dealt with this in ZAP Search Front End by pulling in all the shared components in the consumer repo. This creates duplication, yes, but reduces overheard. I've heard this advice from other organizations dealing with addon-related upgrade woes.
I'm open to suggestions on this. The other option is to "fix" the labs-ui dependency, but I'm concerned with the unnecessary overhead this may create. I'll spike this path to see if it's a possible "fix-all" for consuming repos!
The text was updated successfully, but these errors were encountered: