Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support NodeNext module resolution #15

Merged
merged 3 commits into from
Feb 21, 2023
Merged

support NodeNext module resolution #15

merged 3 commits into from
Feb 21, 2023

Conversation

bbohlender
Copy link
Contributor

The placement of the *.d.ts files currently prevents support for projects that use the NodeNext module resolution since the .d.ts are expected to be placed with their definitions. Additionally, the .js file ending is expected. The changes in this PR should enable both the current node moduleResolution and the NodeNext moduleResolution.

@jeetiss
Copy link
Collaborator

jeetiss commented Feb 14, 2023

@cc-bbohlender could you please remove this line from build.js
https://github.com/shuding/yoga-wasm-web/blob/main/build.js#L44

it doesn't do anything because you commit d.ts files to the repository

@jeetiss
Copy link
Collaborator

jeetiss commented Feb 18, 2023

@cc-bbohlender I'm sorry for the delay

is it will work with yoga's wrapAsm.d.ts? Do we need to change imports in this file too?

https://github.com/facebook/yoga/blob/main/javascript/src_js/wrapAsm.d.ts#L24-L26

@bbohlender
Copy link
Contributor Author

@jeetiss No worries. Thanks for your time :)

I have access to the enums since they are exported here:

export * from "./generated/YGEnums.js";

However, it would be best if yoga's types included the .js file extension.

I can make a PR there too, if you think it's reasonable, they will accept it. There is additionally a problem with the unsetMeasureFun <-- missing a c
https://github.com/facebook/yoga/blob/220d2582c94517b59d1c36f1c2faf5e3f88306f1/javascript/src_js/wrapAsm.d.ts#L159

@jeetiss
Copy link
Collaborator

jeetiss commented Feb 18, 2023

There is additionally a problem with the unsetMeasureFun <-- missing a c

Nice catch, this should be a PR definitely :)

@bbohlender
Copy link
Contributor Author

Okay, thanks, will do.

Do you happen to know why yoga exports the constants on the yoga object?
https://github.com/facebook/yoga/blob/220d2582c94517b59d1c36f1c2faf5e3f88306f1/javascript/src_js/wrapAsm.js#L149

Because the yoga-wasm-web types express that the constants are exported

export * from "./dist/generated/YGEnums";

But the actual implementation does not export them

Resulting in no compile error from typescript but a runtime error for:

import { UNIT_AUTO } from "yoga-wasm-web"

yoga-wasm-web does not provide an export named 'UNIT_AUTO'

I would at least export the constants here:

but I then also, I see no reason to put them on the yoga object
https://github.com/facebook/yoga/blob/220d2582c94517b59d1c36f1c2faf5e3f88306f1/javascript/src_js/wrapAsm.js#L149

@jeetiss
Copy link
Collaborator

jeetiss commented Feb 18, 2023

This is a nice catch too.

this is happening because we initialize wasm differently how original yoga does. i think that we should reuse the original yoga as much as possible so we can migrate when it will be released (i hope it will be some time)

I'm going to export constants too as in the original yoga package

https://github.com/facebook/yoga/blob/main/javascript/src_js/entrypoint/_entryAsync.js#L14
https://github.com/facebook/yoga/tree/main/javascript#usage

@jeetiss jeetiss mentioned this pull request Feb 18, 2023
@jeetiss
Copy link
Collaborator

jeetiss commented Feb 18, 2023

Seems like this is not so easy to do because esbuild can't reexport commonjs imports

@bbohlender
Copy link
Contributor Author

Yeah, I see. I guess, we have to wait until yoga changes the js code to es6 modules.
The constants could be exported as Constants, but for my use case in particular, I don't need them.

However, I would then at least remove the export to prohibit confusion from here:

export * from "./dist/generated/YGEnums";

I will now continue to make the PR on yoga to use the file extensions and include the typo fix. Depending on how fast that PR goes through, the current PR here, is also valuable for me without the enums from yoga.

facebook-github-bot pushed a commit to facebook/yoga that referenced this pull request Feb 21, 2023
Summary:
I wanna repeat the constants export in `yoga-wasm-web`, to achieve

```js
import { ALIGN_CENTER } from "yoga-layout";
```

And I failed. it is impossible because `rollup` and other tools can't transform commonjs `module.exports = { WHATEVER: 1 }` into ECMAScript modules. however, they can work with separate exports like `exports.WHATEVER = 1` and this PR transforms yoga constants into this convertible format

This doesn't change anything for the yoga package, but it makes it possible to reexport constants without any modification and hacks, like this

```js
export * from "./javascript/src_js/generated/YGEnums.js";
```

[discussion in yoga-layout-wasm](shuding/yoga-wasm-web#15)

Pull Request resolved: #1229

Reviewed By: NickGerleman

Differential Revision: D43437177

Pulled By: rshest

fbshipit-source-id: bfe1404d1b48779f404e6510f2aafadd7fd4e774
@jeetiss jeetiss merged commit c47687f into shuding:main Feb 21, 2023
@jeetiss
Copy link
Collaborator

jeetiss commented Feb 21, 2023

@cc-bbohlender thanks for your contribution! 🎉

I have some ideas on how we can modify d.ts files from the yoga repo to support NodeNext. But it will be nice to have tests. Do you have any experience in writing tests for types? i'm not ;) I saw that vitest have added support for testing types https://vitest.dev/api/expect-typeof.html

@bbohlender
Copy link
Contributor Author

@jeetiss After reading the comment from the yoga PR, I see a problem.
facebook/yoga#1228

They are not using the "type" field in their package.json, which defaults to "commonjs". The yoga-wasm-web package uses "type": "module". I guess this is to enable "yoga-wasm-web/asm" import instead of "yoga-wasm-web/dist/asm". However, since yoga-wasm-web uses the files from the yoga repo, their interpretation differs since the "type" field in both use cases differs.

The hard way would be to write declaration files that can be interpreted as "module" and "commonjs" or to try and transition the yoga repo to also use "module".

However, the simplest solution I see is to change this package to "commonjs" and flatten the dist folder in the distribution so that "yoga-wasm-web/asm" can still be imported.

Since "commonjs" packages can be used in NodeNext, this would also make this PR obsolete.
However, I am unsure if other reasons require the "type": "module" field. I am sorry that this solution comes to my mind so late.

@jeetiss
Copy link
Collaborator

jeetiss commented Feb 22, 2023

write declaration files that can be interpreted as "module" and "commonjs"

I don't understand why we should support "commonjs" here, because this fork is in esm (rollup converts all commonjs files from yoga repo into esm), so as I understand from TS docs we need to convert the d.ts files to esm compatible format to. Am I right?

change this package to "commonjs"

I prefer to keep this package as "module"

@bbohlender
Copy link
Contributor Author

From the yoga repo, there is no reason to change the file extensions in the .d.ts files, since everything works fine even with NodeNext since their package type is "commonjs". Since this package has type "module", the .d.ts files must include the file extensions to support NodeNext.
Maybe there is a tool that can automatically add the file extensions, or we write own code to do that, or we include own typings into the repo, which would then diverge from the yoga repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants