Moderated By: drpal

Hello! Welcome to to our weekly JavaScript initiative meeting. It happens every Monday right here in Slack!
- This meeting is chat-only. There is no video or audio component.
- We leave about 5 minutes between topics, sometimes more, so that people who are multitas- The meeting is threaded, so please keep an eye on thread notifications (this may not pop an alert depending on your settings.)
- Please DM @justafish or myself if you would like any items added to today’s agenda

1️⃣ Please introduce yourself if you’re attending today’s meeting!

drpal Matt!
xjm Hi
dawehner Daniel, my name
prestonso Preston So in New York City :smile:
drpal @xjm :wave:
justafish :wave:
mrjmd Matt also, in San Francisco
webchick ’Ello! :slightly_smiling_face: I’m webchick from Vancouver, BC, Canada.

@justafish I didn’t have anything for the agenda. I didn’t know if you got anything?

@drpal nein

let’s go with the last item from last week

Do we want to bikeshed names of things at at all?

2️⃣ Lets talk about the naming of some theme functions from

drpal theme functions!
drpal @justafish I made this.
drpal @webchick The names are a bit clunky, but I think having a decent description helps.
drpal I’m actually not a huge fan of `drupal.theme.message` since it’s pretty vague on whats happening.
webchick So what does themeWrapper do?
justafish just to clarify @drpal - this is only for our jQuery codebase, right?
justafish doesn’t have any bearing on the new stuffs
drpal @justafish Just the current codebase. No fancy React stuff.
drpal @webchick There is `Drupal.theme.message` which is the theme function for the actual messages
drpal And, `Drupal.theme.createMessageInternalWrapper` which creates the location that the JavaScript messages are placed into.
webchick So you said you were not a fan. What do you feel about that isn’t descriptive enough?
dawehner Well I guess this is more imperative than descriptive, right?
dawehner so messageInternalWrapper vs. createMessageInternalWrapper
drpal @webchick I wasn’t a fan of `Drupal.theme.message`
webchick Ah, and nod_ wasn’t a fan of the internalwrapper thingy?
drpal @dawehner Yeah, I did think that dropping `create` would be a good idea.
drpal @webchick Yes, not a fan of `Drupal.theme.createMessageInternalWrapper`
webchick Cool, well messageInternalWrapper would at least make it more consistent with message. But I’m pretty clueless about our general naming conventions in JS code to know if that makes sense overall. :slightly_smiling_face:
drpal Yay! A sane conversation about naming something.
webchick Ok, so sounds like there’s consensus. :slightly_smiling_face: Who should post back?
drpal @webchick I can just roll another patch
webchick I’m happy to do so if no other volunteers. Or do you just want to wrap that (hah) in your next patch?
webchick nice
xjm :rimshot:
dawehner Also wrapper sounds specific to a per theme thing
xjm Hm, yeah indeed
dawehner Still I think the functionality is the wrapper for all of the 3 kind of messages

@webchick threads please :slightly_smiling_face:

@webchick DM @drpal if you have an agenda item for this meeting

Oh, I’m so sorry!

3️⃣ Follow up for (no restricted syntax 1/2)

drpal This probably needs some work.
dawehner Yeah I think droplet makes valid points
dawehner And this really makes it hard to review though
xjm So from what I understand from droplet (which is not much), it sounds like he's indicating some of the replacements change the logic slightly from what's in HEAD
xjm Which is part of what I was also worried about
xjm One of the examples seems to be concern about an empty condition that previously wouldn't have caused an error, but would now, right?
xjm Or where the variable was defined outside of the loop previously
dawehner So yeah we need to be rather safe than sorry
dawehner So for every of those cases do ```|| {}```
drpal Shouldn’t a lot of these be handled with default params?
xjm PHP translation: It's the same issue as if you'd done `foreach` on a `NULL` without initializing to `[]` instead?
drpal Additionally, we’re setting ourselves up for more complex failures by providing empty objects in this case.
dawehner @drpal For review purposes I would suggest a follow up for doing so?
xjm I'm hesitatnt about that.
drpal It’s also pretty difficult to figure out the surrounding context (without actually applying the patch), but I think some of these `|| {}` aren’t really needed.
xjm If there is a risk of breaking BC for odd contrib code, I don't think we should have a followup to explore that
webchick In PHP, we “just” ensure variables are always instantiated before they’re acted on. (to not do so generates a notice.) Can we do the same here?
xjm It's more work, but it might be better to take the APIs one at a time and discuss what inputs and outputs are actually possible? That's something I get lost in ebecause it seems like there's os much in the global state
webchick Because it seems like this would be the equivalent of spewing isset($var) ? … : … everywhere which… ew.
drpal @xjm So we’ll finish this by Drupal 9?
drpal Cool.
xjm @webchick Well in PHP we would ensure that the methods were in a signature or such, preferably
xjm And throw exceptions if they were missing.
webchick Right.
drpal @webchick Thats the point of default params.
drpal In JavaScript.
xjm BUT if we had something that wasn't already fataling in that way, then we'd need to be ccareful
webchick Which is what Matt is saying about… yes.
webchick Honestly? I’m more inclined to do the change, manually test the hell out of it, and then fix whatever bugs we somehow don’t find from there.
xjm So droplet's solution has the risk of swallowing errors that might be thrown
xjm @webchick My concern is less about breaking core than breaking other things
xjm I mean I also don't want to break core :wink:
drpal @xjm There’s also two different patterns used for fixing these issues.
webchick These are coding standards changes, and when they require 10s of issues to put down, it erodes this team’s happiness level sigificantly.
dawehner @xjm That is a strong point, so maybe we would need to talk about each point individually
drpal Some of these are fixed with default params.
drpal Some are fixed with `|| {}`
xjm @webchick If a codinstandards issue is introducing risk, we
xjm shouldn't be doing it the way we are
webchick That’s why we manually test the hell out of them. :slightly_smiling_face:
webchick To help eliminate risk
xjm Manually testing them doesn't take care of when you are providing an API for someone else
xjm We have wontfixed PHP standards cleanups becacuse they are too risky. Or postponed them on other parts.
webchick True. So tests should cover that. And if they don’t, we’ll write a new test if a bug gets uncovered as a result.
xjm So essentially doing what Daniel did for Ajax, for eacch of these
webchick I mean, at one point Drupal 7 had zero test coverage. We still made changes. They sometimes introduced bugs. We fixed them.
webchick I’m confused why we can’t do the same here.
webchick If we were worried about data integrity issues, that’d be one thing.
xjm @webchick D8 is in production
webchick Yes. Which is why it’d be good to get them in sooner than later so we have more months of time to test in HEAD.
xjm I disagree with that
xjm If that were acctually the acse we should only be committing them when 8.6.x is opened. But I don't think these are enough to merit that.
webchick That’s fine. :slightly_smiling_face: I’m trying to strike a balance here between wanting to mitigate risk and also not wanting to burn this team out to a smouldering pulp :smile:
xjm It's worth considering whether someone, *not Matt*, might want to split them off one by one and review eacch and write tests for it.
xjm Or alternately, just choosing not to fix this for legacy APIs.
xjm I don't think it needs to be a big thing
webchick ^ what about that approach? Leave the old crusty code we want to remove in favour of React back in the age of dinosaur coding standards, and make this change going forward to newfangeldy code?
webchick That also came up last meeting as a suggestion iirc
justafish +1 for that
dawehner I still think other eslint rules are worth fixing
xjm Yeah, I think anything that doesn't require refactoring is easy and makes the codebase better
drpal Can we clarify what we’re suggesting?
webchick I think it would be:
xjm For things that require a little refactoring, I also think it's worth doing. Just call it a meta issue and let less advancecd ccontributors take one piecec at a time.
xjm And writing tests like dawehner did.
webchick I think the basic problem is that what @drpal is trying to do is get our code base up to snuff so that JS developers don’t take one look at it and run away.
webchick The sooner we get this done, the better off we are.
webchick Taking each one of these rules and splitting it into N issues, each of which needs its own review/needs work/review/rtbc/needs work/review/rtbc/commit cycle *also* introduces risk.
webchick Risk of prolonged timelines, risk of burnout, risk of the code base looking like a total mishmash in the meantime, etc.
xjm @webchicck I am not saying each of these issues, at all
xjm Nowhere near
webchick It’s a question of which risk we want to optmize for
xjm I'm saying when it involves actually changing logic
dawehner If you really want to attract people don't use Objects in the first place but rather use something like a map
drpal @dawehner Flying in from the future?
drpal :stuck_out_tongue:
dawehner @drpal Isn't Map es6?
dawehner Oh this would require polyfills?
xjm @webchick A dozen or so fixes have gone in that were just fine as one patch, beacuse the changes could be read in isolation
dawehner @drpal
dawehner Are the other issues easier again?
drpal @dawehner I think you’ll need a polyfill for that.
drpal @dawehner Once restricted syntax is wrapped up I wanted to get no-use-before-define done.
xjm When we refactor PHP code, we also add missing tests as part of the issue. Whihc brings us bakc to JS tests being in PHP and the barrier that creates. :stuck_out_tongue:
dawehner @drpal ******* That isn't easier either
drpal That one won’t change any logic, just where things are declared.
dawehner @drpal You haven't automated the rearranging, right?
drpal @dawehner No, I haven’t.
webchick So back on the original topic, which is how do we put that one away, what is the next step here? One issue per change?
webchick Change all of them to the || {} check?
webchick Give up and go play mario kart?
xjm Maybe it's worth 1-2 test balloon issues?
webchick Divided among what lines?
xjm A subsystem, I'd say
xjm E.g. to confirm that the AJAX stuff for the rename issue had BC, Daniel wrote a test for it
drpal @webchick Why don’t I provide another patch that adds `|| {}` checks in all the locations where we switched from a `for...in` loop to a `Object.keys().forEach()` loop.
dawehner @xjm With subsystem you mean file, given that all these changes are mostly one subsystem per file?
drpal Then we can re-review and go from there.
dawehner @xjm So basically you suggest to add test coverage for every change?
xjm I dunno, just trying to brainstorm
dawehner @drpal @xjm made the point earlier that this could hide other errors, but I think we effectively hide this errors as of now already
xjm @drpal What other side effects might there be from adding `|| {}`? You said that was also a concern
xjm Or ah dawehner said it :slightly_smiling_face:
xjm So, jinx
dawehner so we can agree on adding it everywhere?
drpal I think we’re on the same page. :stuck_out_tongue:
webchick If || {} lets us close this in one shot, and we can go back in subsequent refactors + test coverage to figure out on a more granular level which of those we can remove, then +1 from me.
drpal @xjm If you don’t throw an error on a missing object, by doing `|| {}`, and whatever was supposed to happen in the loop doesn’t, because there are no keys to iterate over, then there is a possibility of some very difficult to debug errors.
webchick I think the main thing is to try and “move the needle” on these eslint rules as much as possible in 8.5 so we can a) get this team a nice win, and b) all refocus onto more impactful things.
drpal If we wanted to be extra safe, we could always check that an object exists, and that is is not empty before iterating over it.
webchick @drpal isn’t that what one of the coding standards rules you want to introduce does anyway? Ensure a variable is declared before it’s used?
webchick If so, does it make sense to do that one first?
drpal @webchick `no-use-before-define` doesn’t apply to just variables, functions as well.
drpal However, that would potentially solve some of this.
xjm That's a great idea
drpal I actually don’t think it’s too bad of an idea to throw an error when you’ve done something silly and the object is missing/empty.
drpal @xjm It’s on the list to complete at some point.
xjm Yeah, I think we should TBH... but it's a balance between cchanging to a defensive API, or breaking BC for unintended usecases
dawehner @drpal For reviewing no-use-before-define, would it make sense to split vars up from functions? I know there is a loose distinction between those ...
xjm That would be helpful
drpal @dawehner haha
webchick Vars vs. functions seems like a fairly arbitrary line. Would different people need to review vars vs. functions?
xjm
dawehner @drpal impossibru?
drpal @dawehner technically not impossible, but you know.. thats the best kind right?
drpal @dawehner I will wrap up the changes to 1/2 no-restricted-syntax today.
drpal That way we can review and then move on.
drpal @webchick It’s a little vague. We do a lot of function declaration (v. expression) in our current javascript.
drpal The line is kind of blurred there.
webchick gotcha.
drpal @webchick If you happen to be curious about function declaration v. expressions, :slightly_smiling_face:
webchick Oh, thanks. I was more meaning like. AJAX vs. States vs. Tabledrag vs. etc. … if different people would be reviewing those issues, then it might make sense to split along those lines.
webchick But it seems like JS is just “JS” across the board. In any case, optimizing for patch reviewers since they’re the critical piece in getting anything to resolution.
xjm That is actually super helpful
xjm Well so for the no-restricted-syntax thing with empty values. There are presumably places where it's impossible for thing thing to be empty, because of how the function is written. And there are presumably places where an empty value might have been acccidentally supported in the past.
xjm Even just taking the time to take that explicitly in each case is helpful.
xjm It's just that it's a struggle to know what our public API is, much more so than with PHP.
xjm So changing something that should have failed explicitly to fail silently is bad.
xjm But also changing something that would have failed silently to fail explicitly is a BC break.
xjm So I do think it might be worth reviewing on a case-by-case basis
xjm For PHP we'd be adding either change records or deprecations when the old code path supported things it shouldn't.
xjm Did we come to a conclusion here? @dawehner @drpal Are we going to just do the other rule first, and then this one after?
xjm Because risking BC breaks or swallowing errors that should be raised are both non-deal :slightly_smiling_face:
dawehner While catching up I think having @internal and @api for everything new and maybe existing would be super helpful

total aside: every time i read backscroll in this channel i literally read every message in the voice of the poster. it’s like sitting in on an in-person sprint, in my head, reading these logs

4️⃣ Lets continue the discussion on how we can best support the people who’re maintaining our existing (jQuery) codebase.

drpal I know we only have a few minutes left, but I wanted to get this one out there.
drpal Thanks @justafish :slightly_smiling_face:
webchick Fully agreed. Suggestions? :slightly_smiling_face:
justafish :tumbleweed:
dawehner Suggestion: Get PHP people to write JS tests
webchick Could do. Are there instructions?
xjm Yeah IIRC there isa secction in the handbook
xjm One issue we have is that the few PHP developers who *do* write JS tests reliably spend 5% of their time writing the test and 95% trying to fix the random failyre they introducced
dawehner @xjm Well, we made progress last week with the webdriver issues.
dawehner Review on are welcome, of course
dawehner @xjm There is one question about deprecations in #97 worth having a look at for example
webchick this?
webchick Call me crazy, but it seems pretty bat shit crazy to be testing JS code in PHP. Like. Why don’t we test PHP in Haskell? :smile:
webchick What’s the next step to unblock THAT initiative?
dawehner @webchick Let me repeat what I wrote above: Review on are welcome, of course
webchick Ok great. :slightly_smiling_face:
webchick We have a call with DA folks this week; anything to relay to them that can help expedite things?
dawehner @webchick I don't really think we are blocked on the DA anyone for this issue at the moment. I talk with @mixologic about that stuff more or less regularly anyway
webchick Ok great.
webchick @xjm Should we bump to critical? Seems like we can’t do a lot of these JS initiatives without it.
xjm Not sure about critical. Major, ccertainly.
xjm We *can* write tests for JS functionality, it's just unfriendly.
dawehner @xjm Well its critical as it solves random failures, is that worth doing it?
xjm @dawehner Do you tihnk it will? To me it sounded like it had even more issues depending on scrolling etc. but maybe I misread it.
dawehner @xjm mh, interesting point. I assumes its basically the same. I wasn more talking about phantomjs crashing basically randomly
xjm Yeah most of the random fails in my experience are becacuse someoen tries to interact with an element that's not on the page at the time
dawehner @xjm christian made a suggestion about that on
dawehner But yeah it still requires manual interaction
dawehner @xjm Could we maybe change the size of the viewport by default?
xjm The actual implementation for the test runner is outside what I can give useful feedback on; just know what's happened to date
xjm Posted a comment on the header output thing but I am not sure about much

Comments

justafish created an issue. See original summary.

justafish credited dawehner.

justafish credited drpal.

justafish credited webchick.

justafish credited xjm.

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Status: Active » Fixed
justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Title: Weekly Meeting - 2017/12/04 » [Weekly Meeting] 2017/12/04

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

justafish’s picture

justafish
she/her
Primary language English
Location London, UK
commented
Title: [Weekly Meeting] 2017/12/04 » [Admin UI & JavaScript Modernisation Initiative Weekly Meeting] 2017/12/04
Project: Drupal core - JavaScript Modernization Initiative » Drupal core
Version: » 9.x-dev
Component: Meetings » meetings

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.