Allow bringing arbitrary wing packages from `node_modules`

winglang / wing

A programming language for the cloud ☁️ A unified programming model, combining infrastructure and runtime code into one language ⚡

https://winglang.io

Other

4.73k stars 185 forks source link

Allow bringing arbitrary wing packages from `node_modules` #4975

Open skyrpex opened 7 months ago

skyrpex commented 7 months ago

Feature Spec

Wing allows to bring arbitrary Wing npm packages like this:

bring "@wingcloud/probot" as probot;

new probot.Probot();

The @wingcloud/probot package consists of:

package.json with { "exports": "./src/probot.w" }
src/probot.w with pub class Probot

As you can see, Wing respects the npm exports field.

Use Cases

Allows consuming Wing packages published in NPM that aren't officially endorsed by the @winglibs foundation, and also allows creating Wing monorepos.

Implementation Notes

For implementing the npm exports field, I'm pretty sure there are modern libraries that can help. I'd stick with the modern variants (eg, no main field), but I guess it shouldn't be a huge problem to implement either.

Component

Compiler

Community Notes

Please vote by adding a 👍 reaction to the issue to help us prioritize.
If you are interested to work on this issue, please leave a comment.
If this issue is labeled needs-discussion, it means the spec has not been finalized yet. Please reach out on the #dev channel in the Wing Slack.

skyrpex commented 7 months ago

The workaround for wing monorepos is to bring the relative path (eg, bring "../../../packages/@wingcloud/probot/src/probot.w" as probot; or bring "./node_modules/@wingcloud/probot/src/probot.w" as probot;).

eladb commented 7 months ago

Did you use wing pack to package the library?

I was certain this is supposed to work already!

Chriscbr commented 7 months ago

Essentially this should work (as Elad noted), maybe the open question is how can you use wing libraries in a monorepo without using "wing pack", since the command implicitly modifies your package.json.

@skyrpex Can you try adding "wing": true to the package.json and see if that fixes it?

skyrpex commented 6 months ago

Nope, I didn't use wing pack.

I tried adding "wing": true to the package.json but still doesn't work.

I would've thought that bring would use the node_modules resolution algorithm to look for *.w files, so no special package.json entries would've been necessary. Is it doable?

Chriscbr commented 6 months ago

Nope, I didn't use wing pack.

@skyrpex running wing pack ought to fix your issue. Feel free to ping me on Slack if you have any trouble using it - it should be straightforward.

skyrpex commented 6 months ago

Still an issue... It doesn't work to me, even after running wing pack (which only generates a tarball, right?).

Here's a reproduction repo: https://github.com/skyrpex/wing-monorepo-repro/tree/main.

eladb commented 6 months ago

Why would you need to run wing pack? I think "wing": true should suffice.

skyrpex commented 6 months ago

Why would you need to run wing pack? I think "wing": true should suffice.

Already tried that, too. I just updated the reproduction above.

Chriscbr commented 6 months ago

Got it, it sounds like you don't want to consume the tarball used by wing pack (by linking to it or unzipping it etc). I think to support that use case, the basic issue we need to fix is make the compiler to surface more detailed errors when you try bring-ing an invalid library. These are configuration errors that are automatically fixed in the tarballs produced by wing pack. For example:

error: a library was found, but package.json "name" field doesn't match
error: a library was found, but package.json doesn't contain "version" / "license" / "author" field
error: a library was found, but package.json doesn't contain "wing" field
error: a library was found, but package.json "main" field doesn't point to a valid file

The last item is a requirement to prevent runtime errors if someone tries calling require("your-wing-lib").

skyrpex commented 6 months ago

Interesting. So I added a dummy main entry to package.json ("main": "./index.w" 😄) and it now works.

I wonder, are these restrictions really necessary? Why don't we use traditional node resolution without these requirements?

Chriscbr commented 6 months ago

Why don't we use traditional node resolution without these requirements?

I think the only reason the requirement is in our code right now is because it happens to be a restriction of traditional node module resolution. You can see it documented in the Node.js docs here -- in "LOAD_AS_DIRECTORY" there is a check for a "main" field in package.json. It's also interesting that if you run npm init it automatically provides the "main" field for you.

Ultimately there's nothing saying we have to follow the regular node resolution algorithm though, so if it seems appropriate I'm open to changing our implementation. e.g. we could decide any folder with "package.json" counts as a valid npm package to the Wing compiler. Most of the logic is here.

eladb commented 6 months ago

+1 I don't think we should require the main field for wing libraries, especially given it should point to a .js file, not a .w file, right?

github-actions[bot] commented 4 months ago

Hi,

This issue hasn't seen activity in 60 days. Therefore, we are marking this issue as stale for now. It will be closed after 7 days. Feel free to re-open this issue when there's an update or relevant information to be added. Thanks!

Chriscbr commented 4 months ago

Summarizing action items from this issue -- right now the compiler's modular loader is working mostly as expected. The main things we'd like to change are

Loosen some of the constraints (e.g. allow a module to be loaded even if package.json does not have a "main" field)
Give better diagnostic errors in all other cases that a module was unable to be loaded as a library. See the comment above: https://github.com/winglang/wing/issues/4975#issuecomment-1852225081