Fix Vue, test and document rest #1338
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Update readmes * Fix code style * Add 100% coverage to renderers, babel plugins, loaders, runtime * Add todos inline for potentially broken things * Fix Vue to have tests, support properties, support Vue components * Lock Yarn because latest is broken * Soft deprecate `@mdx-js/util`, `@mdx-js/test-util`, `babel-plugin-html-attributes-to-jsx`: their contents weren’t used or not reused * Set up tests in each package, to allow for different test methods (e.g., vue) * Largely not touching code
wooorm
requested review from
silvenon,
ChristopherBiscardi,
johno and
ChristianMurphy
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/mdx/mdx/ktizvtj21 [Deployment for cce24a6 failed] |
Comment on lines
+60
to
+67
| // To do: is this correct? | ||
| test('should capture renames', () => { | ||
| expect( | ||
| transform( | ||
| 'var name1 = "a", name2 = "b"; export {name1 as foo, name2 as bar}' | ||
| ) | ||
| ).toEqual(['name1', 'name2']) | ||
| }) |
There was a problem hiding this comment.
Good catch.
The names should be aliased as foo and bar
There was a problem hiding this comment.
I guess this package is used more for figuring out which local values are also exported (not caring about as what they’re exported).
mdx/packages/mdx/mdx-hast-to-jsx.js
Lines 153 to 163 in e194fc1
I’m not 100% whether doing it like this, and at build time, is useful? I’d say checking if this was nully at runtime would be better?
mdx/packages/react/src/create-element.js
Lines 23 to 27 in 9443e23
There was a problem hiding this comment.
I think I’ll just leave {import,export}-names and see when I start on mdx-js/mdx whether they can be deprecated
This comment has been minimized.
This comment has been minimized.
ChristianMurphy
requested changes
Nov 17, 2020
* Stop locking dev-dependencies * Move several dev-dependencies from the root `package.json` to the places that depend on them * Remove unused dev-dependencies * Remove unused `yarn.lock`s in non-root * Use `microbundle` for preact, react, and vue * Fix installing, building, formatting, and testing
|
Well, actions is a bit slow but it’s somewhat working. @ChristianMurphy this error might be something for you: https://github.com/mdx-js/mdx/pull/1338/checks?check_run_id=1418253147#step:5:135 And if someone knows why I’m all ears 🤷♂️ |
Not sure, and not too worried, since it's a warning not an error. More concerning is: This might be a network connectivity issue, or it may be an issue with the cache 🤔 |
|
A rerun showed different network timeouts. Maybe what we’re installing is just too much for GH to handle 😅 |
|
Thanks @wooorm, looks like github actions is getting happier. |
|
I’ll see what I can do. I don‘t know Gatsby. FWIW the tests have been failing for a while now https://github.com/mdx-js/mdx/commits/next, I’d rather start moving to smaller PRs |
ChristianMurphy
approved these changes
Nov 19, 2020
Merged
wooorm
added
📚 area/docs
wooorm
added a commit
that referenced
this pull request
Dec 20, 2020
This removes the last three custom Babel plugins we had and replaces them with estree versions. Furthermore, it removes `@babel/generator`. For the plugins, we were only looking at ESM import/exports, but right now we’re delegating work to `periscopic` to look at which things are defined in the top-level scope. It’s a bit more complex, but this matches better with intentions, fixes some bugs, and prepares for a potential future where other ES constructs are allowed, so all in all should be a nice improvement. For serializing, we’re switching to `astring`, and handling JSX for now internally (could be externalized later). `astring` seems fast and is incredibly small, but is not very popular. We might perhaps see bugs is serialization in the future because of that, but all our tests seem fine, so I’m not too worried about that. Estree remains a somewhat fragmented ecosystem, such as that the tree walkers in `periscopic` and `astring` are different, so we might also consider writing our own serializer in the future. Or, when we implement Babel’s React JSX transform ourselves, could switch to another generator, or at least drop the JSX serialization code here. Because of these changes, we can drop `@babel/core` and `@babel/generator` from `@mdx-js/mdx`, which drops the bundle size of from 349kb to 111kb. That’s 68%. Pretty nice. This should improve downloading and parsing time of bundles significantly. Of course, we currently still have JSX in the output, so folks will have to resort to Babel (or `buble-jsx-only`) in another step. For performance, v2 (micromark) was already an improvement over v1. On 1000 simple files totalling about 1mb of MDX: * v1: 3739ms * v2: 2734ms (26% faster) * v2 (w/o babel): 1392ms (63% faster). Of course, this all really depends on what type of stuff is in your MDX. But it looks pretty sweet! ✨ Related to GH-1046. Related to GH-1152. Related to GH-1338. Closes GH-704. Closes GH-1384.
johno
pushed a commit
that referenced
this pull request
Dec 20, 2020
This removes the last three custom Babel plugins we had and replaces them with estree versions. Furthermore, it removes `@babel/generator`. For the plugins, we were only looking at ESM import/exports, but right now we’re delegating work to `periscopic` to look at which things are defined in the top-level scope. It’s a bit more complex, but this matches better with intentions, fixes some bugs, and prepares for a potential future where other ES constructs are allowed, so all in all should be a nice improvement. For serializing, we’re switching to `astring`, and handling JSX for now internally (could be externalized later). `astring` seems fast and is incredibly small, but is not very popular. We might perhaps see bugs is serialization in the future because of that, but all our tests seem fine, so I’m not too worried about that. Estree remains a somewhat fragmented ecosystem, such as that the tree walkers in `periscopic` and `astring` are different, so we might also consider writing our own serializer in the future. Or, when we implement Babel’s React JSX transform ourselves, could switch to another generator, or at least drop the JSX serialization code here. Because of these changes, we can drop `@babel/core` and `@babel/generator` from `@mdx-js/mdx`, which drops the bundle size of from 349kb to 111kb. That’s 68%. Pretty nice. This should improve downloading and parsing time of bundles significantly. Of course, we currently still have JSX in the output, so folks will have to resort to Babel (or `buble-jsx-only`) in another step. For performance, v2 (micromark) was already an improvement over v1. On 1000 simple files totalling about 1mb of MDX: * v1: 3739ms * v2: 2734ms (26% faster) * v2 (w/o babel): 1392ms (63% faster). Of course, this all really depends on what type of stuff is in your MDX. But it looks pretty sweet! ✨ Related to GH-1046. Related to GH-1152. Related to GH-1338. Closes GH-704. Closes GH-1384.
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
/cc @codebender828 for the Vue part, @ChristopherBiscardi for the Preact part, any anyone else who wants to take a look (it’s a bit noisy due to the readme changes)
Hi folks! I added some more docs to readmes and added more tests. I couldn’t get the repo to work (latest yarn seems broken), so I locked that. Most of the work was on the tests, trying to get to 100% coverage. I found a lot of tests that were theoretical (e.g., didn’t evaluate a loaded MDX file). We’re now evaluating more MDX code. Specifically, it seems Vue was (mostly) broken.
And: what’s the
withMDXComponentsin (p)react for? It was never tested, and is only used in an old example in the docs.Changes
private: true)@mdx-js/util,@mdx-js/test-util,babel-plugin-html-attributes-to-jsx: their contents weren’t used or not reused