Skip to content
This repository has been archived by the owner on Jan 13, 2024. It is now read-only.

Support RegExp mountpoints #1121

Merged
merged 10 commits into from
Apr 24, 2021
Merged

Conversation

phated
Copy link
Contributor

@phated phated commented Apr 6, 2021

This adds support for RegExp mountpoints. This can be used like:

process.pkg.mount(/(.+)\.gr$/, function (_match, group1) {
    return path.join(process.cwd(), group1 + '.gr.wasm');
});

This is useful for more complex patterns, like writing files matching the internal snapshot structure to the user's filesystem.

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@leerob leerob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a test, but then good to go!

prelude/bootstrap.js Outdated Show resolved Hide resolved
@phated
Copy link
Contributor Author

phated commented Apr 6, 2021

I added a test, though I don't understand the directory naming so I named it with this PR number (test-1121-regexp-mountpoints).

This also helped me catch a case I overlooked in the readdirMountpoints function, so I hardened that too.

@phated
Copy link
Contributor Author

phated commented Apr 6, 2021

Any idea why the pnpm stuff might be failing in CI?

@robertsLando
Copy link
Contributor

@erossignon

@erossignon
Copy link
Contributor

@erossignon

Any idea why the pnpm stuff might be failing in CI?

pnpm has recently been updated to version 6.0.0 , they seems to have droped support for nodejs <= 10.0

A easy fix would be to force a particular version of pnpm or to ignore the test when nodejs <=10 is detected.

@robertsLando
Copy link
Contributor

I would skip tests for nodejs <= 10

@erossignon
Copy link
Contributor

please merge #1122 first , this will fix the issue

@robertsLando
Copy link
Contributor

@phated
Copy link
Contributor Author

phated commented Apr 7, 2021

Sorry about that! I didn't take into account Windows path separators in my RegExp, but I simplified it and now the tests are passing.

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this someway fix #1075 ?

Copy link
Contributor

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jesec
Copy link
Contributor

jesec commented Apr 8, 2021

Could this someway fix #1075 ?

Very unlikely to be related.

By the way: Forgive me for being the “progress blocker” here. While those changes look good to me, IMO we should ship the new binaries before further feature changes.

@phated
Copy link
Contributor Author

phated commented Apr 9, 2021

@leerob was there anything else you'd want me to do here?

@robertsLando
Copy link
Contributor

@phated Wait for the 5.0.0 release then we can merge this :)

@erossignon
Copy link
Contributor

I think this feature needs to be documented in the README.md.

@phated
Copy link
Contributor Author

phated commented Apr 18, 2021

I think this feature needs to be documented in the README.md.

When I was digging into the project, mountpoints weren't documented at all. Has that changed?

@robertsLando
Copy link
Contributor

@phated Could you add some docs about this feature please? I'm ok for this to land and I know there was no doc for this but I would like to make docs better

Copy link
Contributor

@erossignon erossignon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

requires added documentation in Readme.md

Copy link
Contributor

@jesec jesec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg2m.

As for the documentation, currently there is no documentation for process.pkg.mount at all. I think the burden belongs to us maintainers instead of the author of the PR.

@robertsLando
Copy link
Contributor

I think we can merge this so

@erossignon erossignon self-requested a review April 24, 2021 10:13
@jesec
Copy link
Contributor

jesec commented Apr 24, 2021

@leerob Tests have been added by @phated . Can you re-review?

@erossignon
Copy link
Contributor

@leerob let's go !

@leerob leerob merged commit 382f643 into vercel:master Apr 24, 2021
@phated
Copy link
Contributor Author

phated commented Apr 24, 2021

Thanks all!

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

Successfully merging this pull request may close these issues.

None yet

5 participants