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

Babel plugin absoluteRuntime should support relative paths #10355

Open
arcanis opened this issue Aug 19, 2019 · 7 comments · May be fixed by #10378

Comments

@arcanis
Copy link
Contributor

commented Aug 19, 2019

Feature Request

Is your feature request related to a problem? Please describe.
The path stored because of absoluteRuntime is absolute. As detailed in the doc:

Using absolute paths is not desirable if files are compiled for use at a later time

Describe the solution you'd like
Instead of either full absolute paths or absolutely not resolved bare identifiers, a third option should be relative paths. This would solve the problems of both approaches:

  • The relative path would be guaranteed to work provided that the install location and the transpiled files keep the same hierarchy from a computer to another, which is much easier to do than absolute paths (since you generally don't control where a repository is cloned).

  • The relative path would prevent cross-packages boundary issues when using PnP (PnP would prevent packages that don't depend on runtime-corejs2 to require it, which is problematic in environments such as Next.js which transpile everything).

From an implementation perspective, depending on how safe you would want to play it, there are two possible options:

  • absoluteRuntime could use a relative path by default
  • We could introduce a relativeRuntime field with the same semantic, but relative

Describe alternatives you've considered
Using relative paths is the approach we took for the PnP file itself, and so far it worked really well. I don't see an alternative that would work as well.

@babel-bot

This comment has been minimized.

Copy link
Collaborator

commented Aug 19, 2019

Hey @arcanis! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant Slack community that typically always has someone willing to help. You can sign-up here for an invite.

@loganfsmyth

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Would you be able to elaborate more on the usecase, maybe with a clearer example of how the existing logic is not workable for you? I'm trying to understand the case where you'd want to compile node_modules, but then not be bundling the code in such a way that the path is irrelevant. Are you packaging up the code in a different way?

@arcanis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

My use case is abstract in that it doesn't affect me right now - it's more that I think it would be a problem for other users.

For example, I could see a case for transpiling all the files used by a server application offline for speed before sending it to a deployment server. If the deployment path isn't the same as the one used when transpiling the application, then it would crash.

@loganfsmyth

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

My concern with relativeRuntime is that:

  • the plugin runs on many files with many paths, so there is no one relative path.
  • absoluteRuntime lets us resolve the location of the runtime once when the plugin loads, instead of per-file.

What we could do, which I'd be fine with, is add a useRelativeRuntime boolean option or something, which would do what we do now, but then instead of injecting the absolute path, it would first compute path.relative(jsAbsoluteFilename, runtimeAbsolutePath) and then use that?

@arcanis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

What we could do, which I'd be fine with, is add a useRelativeRuntime boolean option or something, which would do what we do now, but then instead of injecting the absolute path, it would first compute path.relative(jsAbsoluteFilename, runtimeAbsolutePath) and then use that?

Ahah, this is actually what I had in mind with relativeRuntime! 😃

@loganfsmyth

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Cool, sounds good then! Re-reading it, I think I just didn't quite get that from your post. When I read

We could introduce a relativeRuntime field with the same semantic, but relative

I thought you meant, instead of absoluteRuntime there would be relativeRuntime to pass a relative string somehow, which did not seem right.

@arcanis arcanis referenced a pull request that will close this issue Aug 31, 2019
@arcanis

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2019

I've opened #10378 which implements this feature 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.