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

add esbuild config to CLI build and dev #2564

Merged
merged 5 commits into from
Jun 1, 2021
Merged

add esbuild config to CLI build and dev #2564

merged 5 commits into from
Jun 1, 2021

Conversation

thedavidprice
Copy link
Contributor

@thedavidprice thedavidprice commented May 20, 2021

Closes #2069

@peterp decided to keep it simple for this one and go with:

[experimental]
     esbuild = false

What say you?

@thedavidprice thedavidprice marked this pull request as draft May 20, 2021 18:20
@jtoar jtoar added topic/cli topic/config Babel, Webpack, ESLint, Prettier, etc. v1-priority labels May 20, 2021
@jtoar jtoar added this to the next-release-candidate milestone May 20, 2021
@thedavidprice thedavidprice marked this pull request as ready for review May 21, 2021 01:17
@thedavidprice thedavidprice requested a review from peterp May 21, 2021 01:18
@peterp
Copy link
Contributor

peterp commented May 22, 2021

LGTM, do you want to add it to the redwood.toml file?

Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

I've added a suggestion to make it evaluate the default value when the command is executed.

There are also some formatting warnings.

packages/cli/src/commands/build.js Show resolved Hide resolved
packages/cli/src/commands/dev.js Show resolved Hide resolved
@pdjota
Copy link
Contributor

pdjota commented May 25, 2021

@thedavidprice probably would help updating the documentation on the feature https://github.com/redwoodjs/redwoodjs.com/pull/703

@thedavidprice
Copy link
Contributor Author

@peterp

1. Add to CRWA Template redwood.toml

Yes, I think that's a good idea.

  • informs people about experimental features
  • doesn't seem to have a future downside — when we remove the config, it doesn't break anything. If anything it might give us another opportunity to communicate that the feature is now stable.

2. Use option.default()

I spent some time fiddling with this. Functionally it does work. However, it doesn't execute and display the value with the --help option. E.g.:

$ yarn rw build --help
...
rw build [side..]

Build for production
...
      --esbuild    Use ESBuild for api side [experimental] [boolean] [default: (default)]
...

Yargs docs don't offer much help other than that "the function name will be used...". However, I tried changing the function name and it wouldn't build.

I'm assuming the priority is performance — avoid adding init time to Yargs — it was hard to determine the cost of not using the function. In several tests, the command execution time was identical. And we do already use something similar to default: getConfig()... to get Sides for rw test and language target for other commands.

I feel like the correct --help output value is helpful. Unless you have other ideas about how to finagle this, I'd prefer to use my original code.

?

@jtoar jtoar removed the topic/cli label May 28, 2021
@thedavidprice thedavidprice requested a review from peterp May 28, 2021 16:57
@peterp
Copy link
Contributor

peterp commented Jun 1, 2021

@thedavidprice Ok, if that doesn't work then let's just add it by default. I need to make those functions lazy.

@thedavidprice thedavidprice added the release:breaking This PR is a breaking change label Jun 1, 2021
@thedavidprice thedavidprice merged commit 2deda8a into redwoodjs:main Jun 1, 2021
@thedavidprice thedavidprice deleted the dsp-create-esbuild-toml-config branch June 1, 2021 16:56
dac09 added a commit to dac09/redwood that referenced this pull request Jun 2, 2021
…s-ts

# By David Price (6) and others
# Via GitHub
* 'main' of github.com:redwoodjs/redwood:
  build(deps): bump ts-morph from 10.1.0 to 11.0.0 (redwoodjs#2656)
  build(deps): bump core-js from 3.12.1 to 3.13.1 (redwoodjs#2680)
  bump react types and eslint packages patch version (redwoodjs#2695)
  build(deps): bump esbuild from 0.12.1 to 0.12.5 (redwoodjs#2654)
  upgrade misc packages with patch (redwoodjs#2694)
  Fix lerna canary publishing; use default --canary versioning with `git describe` (redwoodjs#2693)
  Upgrade axios due to security alert. (redwoodjs#2688)
  add esbuild config to CLI build and dev (redwoodjs#2564)
  set up yarn offline cache (redwoodjs#2669)
  Create file watch for type def generation (redwoodjs#2614)
  Pin package dependencies, remove CRWA template yarn.lock, set up Yarn offline cache (redwoodjs#2637)
  Fix serve tests (redwoodjs#2668)

# Conflicts:
#	packages/cli/src/commands/generate/types/types.js
#	packages/internal/src/generate/generate-project-typedefs.js
#	packages/internal/src/generate/helpers.js
#	packages/internal/src/generate/templates/scenarios.d.ts.template
dac09 added a commit to dac09/redwood that referenced this pull request Jun 2, 2021
…ter-tests

* 'main' of github.com:redwoodjs/redwood:
  downgrade jest-watch-typeahead 0.6.3 (redwoodjs#2699)
  Exclude yarn packages cache. (redwoodjs#2697)
  build(deps): bump ts-morph from 10.1.0 to 11.0.0 (redwoodjs#2656)
  build(deps): bump core-js from 3.12.1 to 3.13.1 (redwoodjs#2680)
  bump react types and eslint packages patch version (redwoodjs#2695)
  build(deps): bump esbuild from 0.12.1 to 0.12.5 (redwoodjs#2654)
  upgrade misc packages with patch (redwoodjs#2694)
  Fix lerna canary publishing; use default --canary versioning with `git describe` (redwoodjs#2693)
  Upgrade axios due to security alert. (redwoodjs#2688)
  add esbuild config to CLI build and dev (redwoodjs#2564)
  set up yarn offline cache (redwoodjs#2669)
  Create file watch for type def generation (redwoodjs#2614)
  Pin package dependencies, remove CRWA template yarn.lock, set up Yarn offline cache (redwoodjs#2637)
  Fix serve tests (redwoodjs#2668)
  Add script to create a functional test project using the latest CRWA template (redwoodjs#2324)
  build(deps): bump @typescript-eslint/eslint-plugin from 4.24.0 to 4.25.0 (redwoodjs#2627)
  Manage history state length exploding if clicking on a link with with the current route location (redwoodjs#2616)
  [forms] Fix number validation msg (redwoodjs#2552)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change topic/config Babel, Webpack, ESLint, Prettier, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a flag to toml to allow a user to toggle esbuild
4 participants