Rewrite in TypeScript #20

JunichiSugiura · 2020-07-26T09:18:23Z

@jescalan It seems like TypeScript doesn't currently support subpaths exports as described here and here.

One solution is to export everything from index.ts. However it'll introduce breaking changes since every consumer has to change the way to import.

// currently
import hydrate from 'next-mdx-remote/hydrate'
import renderToString from 'next-mdx-remote/render-to-string'

// if we export all from `index.ts`
import {  hydrate, renderToString } from 'next-mdx-remote'

This is not a big library so the change above is acceptable I think but it's up to how you feel about the api change.
Let me know what you think.

hashicorp-cla · 2020-07-26T09:18:26Z

All committers have signed the CLA.

jescalan

Just checked whether the client-side code removal will still work properly when a getServerProps only and a client-side import both come from the same statement and it seems like it does, so I think we can shift the API towards what you suggested 🎉

jescalan · 2020-07-26T14:01:51Z

package.json

@@ -48,14 +52,19 @@
    "next.js"
  ],
  "license": "MPL-2.0",
-  "main": "index.js",
+  "exports": {


This is only supported by node v13 and above if I'm not mistaken, which means we'd be removing LTS node support from this library. Is there any way we can get around this?

jescalan · 2020-07-26T14:05:29Z

src/types.ts

+
+// export interface Components {
+//   [key: string]: React.Component<any>
+// }


JunichiSugiura · 2020-07-28T08:25:14Z

Sounds great! Let me find a time to work on it tomorrow.

jescalan · 2020-07-28T15:29:29Z

Ok I have received another update that it's "not recommended", and should be tested to make sure it works haha

JunichiSugiura · 2020-07-31T07:21:20Z

Sorry, it's been a crazy week for me. I'll reschedule it on Saturday.

JunichiSugiura · 2020-08-01T13:00:04Z

Hmm, I've tested with re-exporting both renderToString and hydrate from index.tsx but it throw "fs" module not found error on the next app side. Which indicates tree shaking doesn't work and renderToString is still included inside client side code. I've also tested with next@9.5 (webpack5 beta enabled) but no luck.

JunichiSugiura · 2020-08-01T13:44:06Z

I found out if I respect current file structure (i.e. hydrate.ts instead of src/hydrate.ts), TS resolves submodules on the app site. I'll remove src/ and dist/.

JunichiSugiura · 2020-08-01T16:18:37Z

@jescalan It should do the trick now.

jescalan

Wow, this is really great. Thanks so much for all the work you put into this! I'm very impressed by the cleanliness of the results. Just had a couple questions as I am not a true typescript pro yet over here.

jescalan · 2020-08-12T21:40:40Z

__tests__/index.js

@@ -5,7 +5,7 @@ const puppeteer = require('puppeteer')
 const handler = require('serve-handler')
 const http = require('http')
 const rmfr = require('rmfr')
-const renderToString = require('../render-to-string')
+const renderToString = require('../render-to-string').default


why is this necessary now?

why is this necessary now?

CommonJS (Node's require() and exports) is not compatible with ES6 default exports. But in this case as we are talking about puppeteer tests I don't see a reason. We are not using TS here.

CommonJS (Node's require() and exports) is not compatible with ES6 default exports

Yes, that's the reason. Without it'll throw TypeError: renderToString is not a function

We can also convert all the test files to TS but I don't have enough time for it.

Does this mean if someone is not using typescript, they will need to pull default in order for the library to work? This seems like a pretty negative change if so, given that the majority of users are likely not on typescript...

jescalan · 2020-08-12T21:41:00Z

babel-plugin-mdx-browser.js

        path.remove()
      },
      // the `makeShortcode` template is nice for error handling but we
      // don't need it here as we are manually injecting dependencies
-      VariableDeclaration(path) {
+      VariableDeclaration: function (path) {


any reason for these additions?

any reason for these additions?

I don't see a reason for these changes.

package.json

jescalan · 2020-08-12T21:43:45Z

render-to-string.ts

+import presetReact from '@babel/preset-react'
+import { renderToString as reactRenderToString } from 'react-dom/server'
+import React from 'react'
+// @ts-ignore


Just curious, why ignore this?

Because the babel plugin module is written in JS and we don't have type definition for it, tsc complains.
I'm not sure where to find type definitions for babel plugins. Maybe someone can help.

I provided babel-plugin-mdx-browser.d.ts: 1341796

jescalan · 2020-08-12T21:45:08Z

render-to-string.ts

+
+  if (!now || !later || !later.code) {
+    throw new Error('Failed to transform mdx source code')
+  }


This doesn't seem like a super useful error message - do you have any more context on when you encountered this and what caused it? Maybe we can add more detail to the error?

The return type of transformAsync is Promise<BabelFileResult | null> and we need to handle the null case. Which is unlikely to happen since it only depends on babel plugins and code const which is returned by mdx(...).

jescalan · 2020-08-12T21:55:04Z

types.ts

+
+export interface Scope {}
+
+export type Components = Parameters<typeof createElement>[1]


This is some crazy wizardry here ✨ - could you help me to understand what's going on? afaik, the Parameters utility creates a type out of the arguments to a function, but you're passing in the results of a typeof operation which I'd think would return "function"?

@jescalan No, the typeof here means the types of the parameters.

For example:

declare function f1(arg: { a: number; b: string }): void; type T3 = Parameters<typeof f1>; // ^ = type T3 = [arg: { a: number; b: string; }]

jescalan · 2020-08-18T21:12:47Z

Just pulled this down to run tests and see if we could get it live - both mdx and babel-preset-react do not have typescript types, and types are not present in the library or definitely-typed for either one. tsc errors on npm test due to implicitAny being set to false.

I tinkered for a bit with trying to get types in for these libraries but ultimately moved on, its not worth the effort for me right now. Let me know if you saw this during your own local dev and how you might want to handle it!

brianespinosa · 2020-08-24T17:10:23Z

Well... @JunichiSugiura you went above and beyond on what I did in my PR. I guess I should have looked to see if any PRs were open doing something similar. 👍🏽 This PR would resolve #24 and then some.

@jescalan let me know if I should decline #26 and instead submit a separate PR for #25

theprobugmaker · 2020-09-01T06:25:55Z

@jescalan I answered a few questions you had on his code.

JunichiSugiura · 2020-09-04T16:44:16Z

I'm sorry it's been a crazy few weeks for me. Let me get back to you tomorrow.

miku86 · 2020-09-12T12:58:56Z

I'm sorry it's been a crazy few weeks for me. Let me get back to you tomorrow.

Hello Junichi,

thanks for your work so far.

Any help required to finish this?

JunichiSugiura · 2020-09-13T08:29:16Z

tsc errors on npm test due to implicitAny being set to false

It's due to @types/ which I had locally wasn't committed. I stupidly run git clean -xdf to reproduce your error and now all the definition files inside the dir are gone🤦‍♂️

I'll recreate these files.

JunichiSugiura · 2020-09-13T14:20:48Z

Sorry for taking it so long. I finally managed to find some time to

reflect some reviews
fix third-party related type errors
merge master

Hope it works this time🤞

jescalan

Thank you again for the great work here @JunichiSugiura! We're getting super close I think

jescalan · 2020-09-14T19:59:17Z

__tests__/index.js

@@ -5,7 +5,7 @@ const puppeteer = require('puppeteer')
 const handler = require('serve-handler')
 const http = require('http')
 const rmfr = require('rmfr')
-const renderToString = require('../render-to-string')
+const renderToString = require('../render-to-string').default


Does this mean if someone is not using typescript, they will need to pull default in order for the library to work? This seems like a pretty negative change if so, given that the majority of users are likely not on typescript...

jescalan · 2020-09-14T19:59:48Z

babel-plugin-mdx-browser.ts

  return {
    visitor: {
      // remove all imports, we will add these to scope manually
-      ImportDeclaration(path) {
+      ImportDeclaration(path: any) {


Does babel not have a type for this one?

These are the types I got working (explanation in the commit):
06kellyjac@d5910b0

I couldn't really find any docs for writing babel plugins in typescript 🤷 so I had to dive through the definitions.
Also it's probably worth double checking the Optional Chaining logic matches before. + that the code removed was actually incorrect.

The tests ran successfully.

jescalan · 2020-12-23T22:23:45Z

I'm going to close this one due to inactivity, and because we have typings shipping out shortly

JunichiSugiura added 5 commits July 25, 2020 19:27

setup typescript compiler

be5600b

rewrite renderToString in TS

6cc0ee4

export as submodules

1826c08

rewrite hydrate in TS

fb42138

exports submodules

ee58d0e

jescalan reviewed Jul 26, 2020

View reviewed changes

JunichiSugiura added 2 commits August 1, 2020 15:07

re-exports from index.tsx

1d7b0b1

cleanup tsconfig

874e7f3

JunichiSugiura added 6 commits August 1, 2020 16:08

mv src/** to /**

bace142

fix emit on release

f59e8ad

typecheck on pre-commit

9ae97ce

allow .js only in __tests__

e20169e

fix test

8c20033

fix test

7c66ed9

JunichiSugiura changed the title ~~[WIP] Rewrite in TypeScript~~ Rewrite in TypeScript Aug 1, 2020

jescalan approved these changes Aug 12, 2020

View reviewed changes

brianespinosa mentioned this pull request Aug 24, 2020

separate member exports and update docs for deprecated mdPlugins option #26

Closed

add definition file for babel-plugin-mdx-browser

4fa0e70

oops, I forgot to disable gitignore for babel-plugin-mdx-browser.d.ts

1341796

JunichiSugiura added 4 commits September 13, 2020 14:47

add definition files for third party libs

3a7f90a

remove unnecessary files

67d5e8e

gitignore babel-plugin-mdx-browser.js

612af4f

Merge remote-tracking branch 'upstream/master' into typescript

87b4497

jescalan reviewed Sep 14, 2020

View reviewed changes

Howard86 mentioned this pull request Oct 11, 2020

Migrate to TypeScript for better types Howard86/next-mdx-remote#1

Closed

jescalan closed this Dec 23, 2020


		export interface Scope {}

		export type Components = Parameters<typeof createElement>[1]

Rewrite in TypeScript #20

Rewrite in TypeScript #20

Conversation

JunichiSugiura commented Jul 26, 2020

hashicorp-cla commented Jul 26, 2020 • edited Loading

jescalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JunichiSugiura commented Jul 28, 2020

jescalan commented Jul 28, 2020

JunichiSugiura commented Jul 31, 2020

JunichiSugiura commented Aug 1, 2020

JunichiSugiura commented Aug 1, 2020

JunichiSugiura commented Aug 1, 2020

jescalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jescalan commented Aug 18, 2020

brianespinosa commented Aug 24, 2020

theprobugmaker commented Sep 1, 2020

JunichiSugiura commented Sep 4, 2020

miku86 commented Sep 12, 2020

JunichiSugiura commented Sep 13, 2020 • edited Loading

JunichiSugiura commented Sep 13, 2020

jescalan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jescalan commented Dec 23, 2020

hashicorp-cla commented Jul 26, 2020 •

edited

Loading

JunichiSugiura commented Sep 13, 2020 •

edited

Loading