Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Repo info
Activity
    Colin Alworth
    @niloc132
    @rdeangelis83 i have a patch that may help, wonder if you would try it and give it a review
    you can copy the modified js (esp in SDM, installScriptDirect.js) into your own project and test that way, see workaround at gwtproject/gwt#9734
    if your bug only happens in SDM, this is likely the cause
    Rocco De Angelis
    @rdeangelis83
    Hi @niloc132 , thx for the fast response. I think my problem is related to:
    https://bugs.chromium.org/p/chromium/issues/detail?id=17356#c107
    https://docs.google.com/document/d/1_GxIGRvv8ATp3GMhA3jUfVFxGKMWEkFWvtqsSS7QQ2M/edit
    In the generated javascript for all method the "displayName" will be added by setting the -XmethodNameDisplayMode ABBREVIATED compiler parameter.
    jsMethod.displayName = "ClassName.methodeName". This needs to replaced somehow now with Object.defineProperty(jsMethod, "name", {value: "ClassName.methodeName"});
    Colin Alworth
    @niloc132
    huh.
    unless i'm mistaken, we now have to do both, not replace jsMethod.displayName, since other browsers ... still exist
    or do other browsers prefer name to displayName, and gwt just stuck with displayName because it implemented the first de facto standard?
    according to OptionMethodNameDisplayMode (the class which handles the arg you mentioned) this function name stuff is for chrome dev tools specifically
    Colin Alworth
    @niloc132
    to patch this, you'll want to modify GenerateJavaScriptAST.outputDisplayName()
    the createAssignment will need to be replaced (... or appended to), with the function call, object decl you described
    Yep
    Colin Alworth
    @niloc132
    exactly
    if you're interested in good debugging (in SDM?), take a look at the patch i linked? someone else noticed that stack traces break in SDM or any "i loaded my gwt module from another origin", and i also noticed that promise events don't work either in this case, but only in chrome
    Rocco De Angelis
    @rdeangelis83
    @niloc132 I'm not 100% sure, but I thing the name property should be supported in must browser. But yes of course we could simply add both
    Colin Alworth
    @niloc132
    the calling code is picky, only accepts an expr statement... wonder if we could nest a comma expr or some garbage like that
    ha looks like we can, that's terrible
    addMethodDefinitionStatement only wants JsExprStmts to be added to it
    and only one, since outputDisplayName is expected to return a single thing
    maybe a second set of statements in the emitMethodImplementation's if check instead
    Rocco De Angelis
    @rdeangelis83
    I could try it out and open a PR but when is the next GWT release planed?
    Colin Alworth
    @niloc132
    i would be happy to push one forward if we get these changes in - i'll review and iterate with you if you can also take a look at my review above?
    would like to check back in with @tbroyer, last i heard he was close to a satisfactory way of starting groupId migration, but i never heard back on how that went
    Rocco De Angelis
    @rdeangelis83
    Okay I will build tomorrow a local version and give your changes a try and give you feedback ...
    Colin Alworth
    @niloc132

    @rdeangelis83

        private void emitMethodImplementation(JMethod method, JsNameRef functionNameRef,
            JsExprStmt methodDefinitionStatement) {
          addMethodDefinitionStatement(method, methodDefinitionStatement);
    
          if (shouldEmitDisplayNames()) {
            addMethodDefinitionStatement(method, outputDisplayName(functionNameRef, method));
            addMethodDefinitionStatement(method, outputFunctionNameProperty(functionNameRef, method));
          }
        }
    
        private void generatePrototypeDefinitionAlias(JMethod method, JsName alias) {
          JsName polyName = polymorphicNames.get(method);
          JsExpression bridge = JsUtils.createBridge(method, polyName, topScope);
          // Aliases are never property accessors.
          generatePrototypeAssignment(method, alias, bridge, JsMemberType.NONE);
        }
    
        private JsExprStmt outputDisplayName(JsNameRef function, JMethod method) {
          JsNameRef displayName = new JsNameRef(function.getSourceInfo(), "displayName");
          displayName.setQualifier(function);
          String displayStringName = getDisplayName(method);
          JsStringLiteral displayMethodName =
              new JsStringLiteral(function.getSourceInfo(), displayStringName);
          return createAssignment(displayName, displayMethodName).makeStmt();
        }
    
        private JsExprStmt outputFunctionNameProperty(JsNameRef function, JMethod method) {
          String displayStringName = getDisplayName(method);
          JsStringLiteral displayMethodName =
              new JsStringLiteral(function.getSourceInfo(), displayStringName);
          JsObjectLiteral descriptor = JsObjectLiteral.builder(function.getSourceInfo())
                  .add("value", displayMethodName)
                  .build();
          JsStringLiteral prop = new JsStringLiteral(function.getSourceInfo(), "name");
          return constructInvocation(function.getSourceInfo(), "Object.defineProperty", function, prop, descriptor).makeStmt();
        }

    try that out, i think it is close (plus or minus formatting)

    Thomas Broyer
    @tbroyer
    @niloc132 All my experiments and conclusions are at https://github.com/tbroyer/gwt-relocation-tests
    Colin Alworth
    @niloc132
    i see early conclusions, do you have final ones or advice?
    Thomas Broyer
    @tbroyer
    Apparently it needed more tests: https://groups.google.com/g/google-web-toolkit-contributors/c/L2RMqglOEXo/m/kCNHSaMeBwAJ (as noted in the project readme too)
    Colin Alworth
    @niloc132
    right, that's what i was intending to allude to, that we were close but not complete to a proposal
    needing more tests in gradle at least suggests someone proficient in gradle (i have done a lot of work since we last talked, but still consider it to be too complex to learn without already understanding it)
    i am not aware of any significant sbt usage of maven, and only minimal ivy usage
    Thomas Broyer
    @tbroyer
    Ha, that experiment-5 looked like those "more tests" actually; and prompted doing an experiment-6 deploying Gradle Module Metadata for GWT itself to confirm the early conclusions (so that gwt-user brings a "dependency constraint" on gwt-dev; similar to having the GWT BOM being transitively depended on). There would still be issues for Maven users not using the GWT BOM, that would have to be dealt with "manually" (using exclusions).
    But experiment-4 could be regarded as "enough" (particularly for a RC, for instance), or even experiment-3 (experiment-4 actually adds an o.g→c.g.g relocation for older versions, so can be added later)
    Colin Alworth
    @niloc132
    ok - so to briefly summarize (and possibly glossing over details), the "missing final post" to the thread could be assumed to say something like:
    • with proposal-5, the gradle experience is acceptable
    • maven requires the use of a bom to be sane, we will need to aggressively push this to any projects that updates either its gwt dependencies or any libraries that it uses, so we need to enlist library publishers to push the bom
    oh, why drop back to 3 or 4 if 5 is strictly better for the gradle cases?
    Thomas Broyer
    @tbroyer
    5 is actually the same as 4 as far as GWT is concerned (it adds Gradle Module Metadata to "third-party" libraries), and 4 is the same as 3 as far as deploying a new version of GWT is concerned (it adds relocations from o.g to c.g.g for older versions)
    Colin Alworth
    @niloc132
    right, that's what i'm trying to say, isnt 5 > 4 > 3? why suggest that we use 3 or 4 as "enough"?
    Thomas Broyer
    @tbroyer
    "6" would add *.module files to the GWT artifacts
    Well, 5 is "exercising" 4 with some external libraries; so as far as GWT is concerned, we would be "doing 4". And "doing 4" requires "doing 3" first (or next, actually).
    In terms of setting up the deployment for a new version, the aim is experiment-3 (it's identical experiment-4 and experiment-5). The additions of experiment-4 could be done separately to "make things better".
    And iterating from that, adding Gradle Module Metadata (similar to what's been proposed for Guava, except we're not interested in the variants here: https://github.com/google/guava/pull/3683/files) implies releasing a new version (it shouldn't stop us doing a first RC without them though)
    Colin Alworth
    @niloc132
    aside from a few extra lines in the bom for the extra relocations, is there a downside for 4? It seems like getting it out as a snapshot or RC with the ability to test mixes 2.7/2.8/2.9 dependencies/projects would be a benefit, rather than adding it later?
    i readily admit to not understanding the implications of 5
    Thomas Broyer
    @tbroyer
    🤔 which "few extra lines in the bom for the extra relocations"? I'm diffing 3 and 4 the only changes are the addition of org.gwtproject 2.9.0 version (and related change in maven-metadata)
    Colin Alworth
    @niloc132
    then i misunderstood the summary (tried to avoid reading the code but stick to the summary and conclusion, to not get caught up in the weeds beyond "does it solve the problems we anticipated")

    https://github.com/tbroyer/gwt-relocation-tests#experiment-4 says

    ...with an additional relocation from org.gwtproject to com.google.gwt for version 2.9.0

    which seems like "extra relocations", no?

    oh i see, not extra relocations, but extra releases
    Thomas Broyer
    @tbroyer
    Yes, those are "new" org.gwtproject:gwt:2.9.0, org.gwtproject:gwt-user:2.9.0 and org.gwtproject:gwt-dev:2.9.0 artifacts; that relocates to their com.google.gwt alter egos
    Colin Alworth
    @niloc132
    so, a library that supports 2.9.0 and isnt ready for 2.10.0 could still move groupid to make the future path simpler
    s/library/project/