Skip to content
Snippets Groups Projects

Support for JSX pragma comment syntax

1 unresolved thread

Fixes #195

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • 581cddf3 - feat: Support for JSX pragma comment syntax (#195)

    Compare with previous version

  • The CI tests failed because some rollup dependency must have been upgraded that broke the ancient version of rollup used in package.json. Could you please upgrade all rollup* related modules in package.json?

    https://gitlab.com/Rich-Harris/buble/blob/f6b9b9dd3e78dd54775a98c862002e17fc4a7ca2/package.json#L57-61

  • Maybe this should be upgraded as well:

    https://gitlab.com/Rich-Harris/buble/blob/f6b9b9dd3e78dd54775a98c862002e17fc4a7ca2/package.json#L50

    I guess an older version of Buble is needed to bootstrap building buble.

  • Hmm... seems like a recipe for merge-conflicts when someone else performs a similar update in another PR or on master.

    IMO this should be updated directly on master - perhaps in a different PR - and once that has been merged, this PR (and others) should automatically pass. Right?

    Edited by username-removed-1297041
  • seems like a recipe for merge-conflicts when someone else performs a similar update in another PR or on master.

    There's not exactly a stampede of pending pull requests. It's fine to fix the issue in this PR. Otherwise you may be waiting for 6 months to get it merged.

  • Ugh. Updating the buble, rollup and rollup-plugin-* dependencies silences the build errors when I'm running node stable (but fails on node LTS:boron).

    However, this suddenly causes 40 or so tests to fail.

    Need to investigate more..

  • If upgrading rollup and buble is more complicated than merely changing package.json, then by all means please make another PR for it. I vaguely recall that GitLab supports some sort of PR dependency ordering - pipelines perhaps?

    If you cannot upgrade to latest rollup and buble, please try just incrementally bumping the versions until you find something that works. Then leave a note open a new Issue for Rich to sort it out.

    Edited by username-removed-568470
  • It will also require minor change to rollup.config.js but not much more, I think.

    Finding the correct combo of dependency versions will take some time.

    ...and the sad part is that even setting all deps to a fixed version will not suffice, as those dependencies have their own ranged dependencies that may have broken upstream.

  • Don't upgrade beyond magic-string@0.15.2 - newer versions have a known bug related to Buble: https://gitlab.com/Rich-Harris/buble/issues/160

    Edited by username-removed-568470
  • @maranomynet did you have chance to get any further with this? Is there anything I could help you with to get this shipped? We could very much do with this at Qubit!

  • @oliverwoodings No I haven't got around to spend time on resolving the outdated project dependencies.

    Point is though, that problem is not really limited to this Pull Request, but is a generic problem with the project as is - and will affect all existing (unmerged) PRs once merged, and all new PRs until this is resolved.

    So resolving this is a general blocking issue for publishing a new version of Bublé.

  • I'm still of the opinion this should be resolved in a separate high-priority PR - which then should be merged before merging and CI-testing any other open PRs

  • Found the source of the problem: a regression in vlq@0.2.2 which was required by magic-string.

    Rant: pkg.dependency version-ranges are dangerous.

    I've created a standalone pull request !120 (closed) ...and rebased this PR on top of that branch.

    Edited by username-removed-1297041
  • added 2 commits

    • 9d0b4f9c - fix: build:browser failed because of regression in vlq@0.2.2
    • cb3d6c1d - feat: Support for JSX pragma comment syntax (#195)

    Compare with previous version

  • FYI - Babel: Removed the deprecated jsx pragma detection code

    https://github.com/babel/babel/pull/6145

  • WAT? This feature is so incredibly useful when using non-react hyperscript libraries - especially if a project happens to use more than one hyperscript library.

  • Noticed it on https://twitter.com/babeljs .

    This sort of thing happens when a feature is not in the ECMAScript standard - and the rest of JSX for that matter.

  • The removal of the /* @jsx override */ pragma from babel@7 was unintentional:

    https://github.com/babel/babel/issues/6141#issuecomment-326624560

  • This thing should be part of the JSX spec, if there is such a thing. I'm much relieved to see this was a mistake.

  • No mention of the @jsx pragma here: https://facebook.github.io/jsx/

    My only point is that people are building code against an unofficial standard that is subject to change and will not be part of ECMAScript.

    If you think about it, this pragma would not work in Rollup if various source files with different @jsx pragmas were combined. The first comment pragma found anywhere in the file would "win". Such a pragma should be either module-scoped or block-scoped. No matter - it clearly hasn't affected anyone.

  • this pragma would not work in Rollup if various source files with different @jsx pragmas were combined.

    I just tried this locally and it does in fact work very well with Rollup.

    Rollup passes the source code of each imported module to bublé before attempting to combine, bundle and tree-shake them, so the @jsx pragma is naturally module-scoped.

    Edited by username-removed-1297041
  • If each module is individually transpiled with Buble or Babel, sure. But if JSX were a first class construct in ES then different @jsx pragmas would not work with scope hoisting in the context of a single file:

    https://github.com/rollup/rollup/issues/1512

    The first @jsx pragma found anywhere in a given file dictates what it'll be:

    $ cat jsx.js 
    f(<A/>);
    /** @jsx Bar */
    f(<B/>);
    /** @jsx Foo */
    f(<C/>);
    $ babel jsx.js
    "use strict";
    
    f(Bar(A, null));
    /** @jsx Bar */
    f(Bar(B, null));
    /** @jsx Foo */
    f(Bar(C, null));
  • Right, I misunderstood your point.

  • Regarding #1512, one should expect the odd unexpected thing to start happening when bundling non-standard ick like JSX as is into single inlined module scope. :-)

  • Please register or sign in to reply
    Loading