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.

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!

packages/babel-cli/src/babel/watcher.ts Show resolved Hide resolved
packages/babel-cli/src/babel/watcher.ts Outdated Show resolved Hide resolved
packages/babel-cli/src/babel/watcher.ts Outdated Show resolved Hide resolved
packages/babel-cli/src/babel/watcher.ts Show resolved Hide resolved
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