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

Allow plugins/presets to indicate external dependencies #14065

Merged
merged 13 commits into from
Feb 2, 2022

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Dec 20, 2021

Q                       A
Fixed Issues? Fixes #8497, closes #11741
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This PR is an alternative to #11741, addressing the concerns I explained in my review:

  • it avoids storing external dependencies as global state, but tracks them locally for each babel.transform call. The caller is then free to merge/overwrite the external dependencies between different calls as it wish.
  • since external dependencies are babel.transform-local, api.addExternalDependency only needs to take the dependency path as a parameter.

Additionally, I made a few other design choices:

  • @babel/core does not resolve external dependencies: they can be any string you want. This makes it possible, for example, to also store URLs or pointers to a virtual fs (as long as the caller knows how to handle them).
  • When a plugin calls api.addExternalDependency, it must also explicitly configure the cache (for example, with cache.invalidate or cache.using). This is because when you have external dependencies you rely on a side communication channel that Babel does not control, so you have to manually invalidate the cache when it changes.

@vedantroy thanks for your awesome work in #11741! I made these changes by myself because I noticed you asked us to do so and I changed almost all the code you initially wrote, but your original design (especially regarding @babel/cli) helped me a ton figuring out what is the best way to integrate this feature in Babel 😄

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 20, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50985/

@nicolo-ribaudo nicolo-ribaudo force-pushed the external-deps-alternative branch 4 times, most recently from fefc840 to 2929ca9 Compare December 21, 2021 18:35
@goatandsheep
Copy link

Just a heads up: with this design, since I maintain a babel plugin that checks .env files, I don't think this will help since I can't tell it to invalidate constantly. Adding external dependencies is a feature that really helps prevent excessive cache clearing

@nicolo-ribaudo
Copy link
Member Author

You can do this:

function plugin(api) {
  const dotEnvFilename = `${process.cwd()}/.env`;

  const env = api.cache.using(() => fs.readFileSync(dotEnvFilename, "utf8"));
  api.addExternalDependency(dotEnvFilename);
  
  return {
    visitor: {}...
  }
}

The api.addExternalDependency(dotEnvFilename) line tells to whatever is calling Babel (for example, @babel/cli) that it needs to be re-triggered when .env changes; const env = api.cache.using(() => fs.readFileSync(dotEnvFilename, "utf8")); reads the .env file and re-instantiates the plugin whever it changes.

@nicolo-ribaudo nicolo-ribaudo force-pushed the external-deps-alternative branch from 2929ca9 to 3165f26 Compare January 10, 2022 22:24
error +=
`Plugins/presets should configure their cache to be invalidated when the external ` +
`dependencies change, for example using \`api.cache.invalidate(() => ` +
`+statSync(filepath).mtime)\` or \`api.cache.never()\`\n` +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
`+statSync(filepath).mtime)\` or \`api.cache.never()\`\n` +
`statSync(filepath).mtimeMs)\` or \`api.cache.never()\`\n` +

@@ -206,6 +221,7 @@ export default gensync<(inputOpts: unknown) => ResolvedConfig | null>(
return {
options: opts,
passes: passes,
externalDependencies: new ReadonlySet(externalDependencies),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it suffices to use TS ReadOnlyArray for externalDependencies, since eventually they would be added to a Set.

@nicolo-ribaudo nicolo-ribaudo added this to the v7.17.0 milestone Jan 11, 2022
@nicolo-ribaudo nicolo-ribaudo added PR: Needs Docs PR: Needs Review A pull request awaiting more approvals labels Jan 11, 2022
Copy link
Member

@sosukesuzuki sosukesuzuki left a comment

Choose a reason for hiding this comment

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

I commented some nits!

@nicolo-ribaudo nicolo-ribaudo force-pushed the external-deps-alternative branch from 9d1a684 to e92d85e Compare January 30, 2022 21:50
@nicolo-ribaudo nicolo-ribaudo removed the PR: Needs Review A pull request awaiting more approvals label Feb 2, 2022
@nicolo-ribaudo nicolo-ribaudo merged commit 7d63d2f into babel:main Feb 2, 2022
@nicolo-ribaudo nicolo-ribaudo deleted the external-deps-alternative branch February 2, 2022 17:22
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli pkg: core PR: Needs Docs PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow plugins to indicate dependencies on random files
6 participants