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

feat(template): add vite-typescript template #3178

Merged
merged 24 commits into from
Aug 17, 2023

Conversation

caoxiemeihao
Copy link
Member

@caoxiemeihao caoxiemeihao commented Mar 3, 2023

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

TODO

  • Write test

@caoxiemeihao caoxiemeihao requested a review from a team as a code owner March 3, 2023 12:45
@caoxiemeihao caoxiemeihao changed the title feat(template): add vite-typescript template wip(template): add vite-typescript template Mar 3, 2023
@caoxiemeihao caoxiemeihao changed the title wip(template): add vite-typescript template feat(template): add vite-typescript template Mar 3, 2023
@caoxiemeihao caoxiemeihao force-pushed the vite-typescript branch 2 times, most recently from 73853b4 to 5e59e3a Compare March 4, 2023 10:00
@erickzhao erickzhao requested a review from a team March 7, 2023 18:19
@tuul-wq
Copy link

tuul-wq commented Mar 20, 2023

Any news when this is going to be reviewed/merged?
Does it slow down release 6.1.0?

@caoxiemeihao
Copy link
Member Author

image

Is the webpack-typescript template test failing?

@GitMurf
Copy link

GitMurf commented Apr 11, 2023

any updates on this? Thanks!

@BlackHole1
Copy link
Member

image

Is the webpack-typescript template test failing?

I checked and it seems to be caused by the dependencies not being installed correctly.

  1. should initially pass the linting process
  1. should pass

@caoxiemeihao
Copy link
Member Author

Sorry, I made a rebase 😅

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@BlackHole1
Copy link
Member

The test case failed. Can you fix it?

@caoxiemeihao
Copy link
Member Author

  • TODO: try to fix test!

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 17, 2023

It's success in my local machine 🤔, how I do to re-run the CI test.
image

@BlackHole1
Copy link
Member

BlackHole1 commented Apr 17, 2023

@caoxiemeihao Try to execute:

npm run clean
git clean -Xfd
yarn install --frozen-lockfile
yarn build
export CI=true
yarn test:slow

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Apr 17, 2023

Still success 🤔 and maybe I need to test the case on Unbuntu 👀

Click show logs
➜  electron-forge git:(vite-typescript) npm run clean
git clean -Xfd
yarn install --frozen-lockfile
yarn build
export CI=true
yarn test:slow

> [email protected] clean
> rimraf dist && lerna exec -- rimraf dist tsconfig.tsbuildinfo

lerna notice cli v6.0.3
lerna info Executing command in 38 packages: "rimraf dist tsconfig.tsbuildinfo"
lerna success exec Executed command in 38 packages: "rimraf dist tsconfig.tsbuildinfo"
Removing .husky/_/
Removing node_modules/
Removing packages/api/cli/node_modules/
Removing packages/api/cli/tsconfig.json
Removing packages/api/core/index.ts
Removing packages/api/core/node_modules/
Removing packages/api/core/tsconfig.json
Removing packages/external/create-electron-app/index.ts
Removing packages/external/create-electron-app/node_modules/
Removing packages/external/create-electron-app/tsconfig.json
Removing packages/maker/appx/index.ts
Removing packages/maker/appx/node_modules/
Removing packages/maker/appx/tsconfig.json
Removing packages/maker/base/index.ts
Removing packages/maker/base/node_modules/
Removing packages/maker/base/tsconfig.json
Removing packages/maker/deb/index.ts
Removing packages/maker/deb/node_modules/
Removing packages/maker/deb/tsconfig.json
Removing packages/maker/dmg/index.ts
Removing packages/maker/dmg/node_modules/
Removing packages/maker/dmg/tsconfig.json
Removing packages/maker/flatpak/index.ts
Removing packages/maker/flatpak/node_modules/
Removing packages/maker/flatpak/tsconfig.json
Removing packages/maker/pkg/index.ts
Removing packages/maker/pkg/node_modules/
Removing packages/maker/pkg/tsconfig.json
Removing packages/maker/rpm/index.ts
Removing packages/maker/rpm/node_modules/
Removing packages/maker/rpm/tsconfig.json
Removing packages/maker/snap/index.ts
Removing packages/maker/snap/node_modules/
Removing packages/maker/snap/tsconfig.json
Removing packages/maker/squirrel/index.ts
Removing packages/maker/squirrel/node_modules/
Removing packages/maker/squirrel/tsconfig.json
Removing packages/maker/wix/index.ts
Removing packages/maker/wix/node_modules/
Removing packages/maker/wix/tsconfig.json
Removing packages/maker/zip/index.ts
Removing packages/maker/zip/node_modules/
Removing packages/maker/zip/tsconfig.json
Removing packages/plugin/auto-unpack-natives/index.ts
Removing packages/plugin/auto-unpack-natives/tsconfig.json
Removing packages/plugin/base/index.ts
Removing packages/plugin/base/node_modules/
Removing packages/plugin/base/tsconfig.json
Removing packages/plugin/compile/index.ts
Removing packages/plugin/compile/node_modules/
Removing packages/plugin/compile/tsconfig.json
Removing packages/plugin/electronegativity/index.ts
Removing packages/plugin/electronegativity/node_modules/
Removing packages/plugin/electronegativity/tsconfig.json
Removing packages/plugin/fuses/index.ts
Removing packages/plugin/fuses/node_modules/
Removing packages/plugin/fuses/tsconfig.json
Removing packages/plugin/local-electron/index.ts
Removing packages/plugin/local-electron/node_modules/
Removing packages/plugin/local-electron/tsconfig.json
Removing packages/plugin/vite/index.ts
Removing packages/plugin/vite/node_modules/
Removing packages/plugin/vite/tsconfig.json
Removing packages/plugin/webpack/index.ts
Removing packages/plugin/webpack/node_modules/
Removing packages/plugin/webpack/tsconfig.json
Removing packages/publisher/base/index.ts
Removing packages/publisher/base/node_modules/
Removing packages/publisher/base/tsconfig.json
Removing packages/publisher/bitbucket/index.ts
Removing packages/publisher/bitbucket/node_modules/
Removing packages/publisher/bitbucket/tsconfig.json
Removing packages/publisher/electron-release-server/index.ts
Removing packages/publisher/electron-release-server/node_modules/
Removing packages/publisher/electron-release-server/tsconfig.json
Removing packages/publisher/github/index.ts
Removing packages/publisher/github/node_modules/
Removing packages/publisher/github/tsconfig.json
Removing packages/publisher/nucleus/index.ts
Removing packages/publisher/nucleus/node_modules/
Removing packages/publisher/nucleus/tsconfig.json
Removing packages/publisher/s3/index.ts
Removing packages/publisher/s3/node_modules/
Removing packages/publisher/s3/tsconfig.json
Removing packages/publisher/snapcraft/index.ts
Removing packages/publisher/snapcraft/node_modules/
Removing packages/publisher/snapcraft/tsconfig.json
Removing packages/template/base/index.ts
Removing packages/template/base/node_modules/
Removing packages/template/base/tsconfig.json
Removing packages/template/vite-typescript/index.ts
Removing packages/template/vite-typescript/tsconfig.json
Removing packages/template/vite/index.ts
Removing packages/template/vite/tsconfig.json
Removing packages/template/webpack/index.ts
Removing packages/template/webpack/tsconfig.json
Removing packages/tsconfig.json
Removing packages/utils/core-utils/index.ts
Removing packages/utils/core-utils/node_modules/
Removing packages/utils/core-utils/tsconfig.json
Removing packages/utils/test-utils/index.ts
Removing packages/utils/test-utils/tsconfig.json
Removing packages/utils/types/index.ts
Removing packages/utils/types/node_modules/
Removing packages/utils/types/tsconfig.json
Removing packages/utils/web-multi-logger/index.ts
Removing packages/utils/web-multi-logger/tsconfig.json
yarn install v1.22.19
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
warning "@aws-sdk/client-s3 > @aws-sdk/[email protected]" has unmet peer dependency "@aws-sdk/signature-v4-crt@^3.31.0".
warning " > @malept/[email protected]" has unmet peer dependency "eslint-plugin-ava@^12.0.0".
[5/5] 🔨  Building fresh packages...
$ rimraf node_modules/.bin/*.ps1 && ts-node ./tools/gen-tsconfigs.ts && ts-node ./tools/gen-ts-glue.ts
$ husky install
husky - Git hooks installed
✨  Done in 35.86s.
yarn run v1.22.19
$ tsc -b packages
$ ts-node tools/test-dist
✨  Done in 84.39s.
yarn run v1.22.19
$ xvfb-maybe cross-env LINK_FORGE_DEPENDENCIES_ON_INIT=1 TS_NODE_PROJECT='./tsconfig.test.json' TEST_SLOW_ONLY=1 TS_NODE_FILES=1 mocha './tools/test-globber.ts'


  WebpackTypeScriptTemplate
    ✔ should succeed in initializing the typescript template (202953ms)
    ✔ should ensure js source files from base template are removed
    template files are copied to project
      ✔ tsconfig.json should exist
      ✔ .eslintrc.json should exist
      ✔ forge.config.ts should exist
      ✔ webpack.main.config.ts should exist
      ✔ webpack.renderer.config.ts should exist
      ✔ webpack.rules.ts should exist
      ✔ webpack.plugins.ts should exist
      ✔ src/index.ts should exist
      ✔ src/renderer.ts should exist
      ✔ src/preload.ts should exist
    lint
      ✔ should initially pass the linting process (3137ms)
    package
      ✔ should pass (11733ms)


  14 passing (4m)

✨  Done in 237.66s.

@BlackHole1
Copy link
Member

BlackHole1 commented Apr 18, 2023

I wrote a Dockerfile based on an existing CI:

FROM ubuntu:20.04

RUN apt-get update && apt-get install -y \
    git \
    curl \
    ca-certificates \
    libssl-dev \
    xvfb \
    zip \
    unzip \
    python3

ENV NVM_DIR /usr/local/nvm
ENV NODE_VERSION 14.21.3
RUN mkdir -p ${NVM_DIR}

RUN curl --silent -o- https://raw.githubusercontent.com/creationix/nvm/v0.39.3/install.sh | bash

RUN . ${NVM_DIR}/nvm.sh \
    && nvm install ${NODE_VERSION} \
    && nvm alias default ${NODE_VERSION} \
    && nvm use default

ENV NODE_PATH $NVM_DIR/v$NODE_VERSION/lib/node_modules
ENV PATH $NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH

RUN npm install -g yarn

# --------

ENV CI=true
ENV DEBUG=electron-installer-snap:snapcraft

WORKDIR /app

COPY . .

RUN yarn install --frozen-lockfile

RUN yarn build

RUN yarn test:slow -g "WebpackTypeScriptTemplate"

图片

But there were no errors reported and it still worked. 🙃

@BlackHole1
Copy link
Member

BlackHole1 commented Apr 18, 2023

OK. I successfully reproduced the problem

In the project create Dockerfile file:

FROM ubuntu:20.04

RUN apt-get update && apt-get install -y \
    git \
    curl \
    ca-certificates \
    libssl-dev \
    xvfb \
    zip \
    unzip \
    python3

ENV NVM_DIR /usr/local/nvm
ENV NODE_VERSION 14.21.3
RUN mkdir -p ${NVM_DIR}

RUN curl --silent -o- https://raw.githubusercontent.com/creationix/nvm/v0.39.3/install.sh | bash

RUN . ${NVM_DIR}/nvm.sh \
    && nvm install ${NODE_VERSION} \
    && nvm alias default ${NODE_VERSION} \
    && nvm use default

ENV NODE_PATH $NVM_DIR/v$NODE_VERSION/lib/node_modules
ENV PATH $NVM_DIR/versions/node/v$NODE_VERSION/bin:$PATH

RUN npm install -g yarn

# --------

ENV CI=true
ENV DEBUG=electron-installer-snap:snapcraft

WORKDIR /app

COPY . .

RUN yarn install --frozen-lockfile

RUN yarn build
docker build -t x .
docker run --rm -it x bash

# In the docker container
 npx xvfb-maybe cross-env LINK_FORGE_DEPENDENCIES_ON_INIT=1 TS_NODE_PROJECT='./tsconfig.test.json' TEST_SLOW_ONLY=1 TS_NODE_FILES=1 mocha 'packages/template/vite-typescript/test/ViteTypeScriptTemplate_spec_slow.ts' 'packages/template/webpack-typescript/test/WebpackTypeScript_spec_slow.ts'

It looks like some operation in ViteTypeScriptTemplate_spec_slow.ts is polluting the WebpackTypeScript_spec_slow.ts

@caoxiemeihao
Copy link
Member Author

Sorry, to be lazy I only tested the WebpackTypeScript_spec_slow.ts file, if it is tested together with ViteTypeScriptTemplate_spec_slow.ts it will reproduce the error 😥

@GitMurf
Copy link

GitMurf commented Apr 20, 2023

The progress looks great here! Thanks for all the hard work! I know the PR isn't closed yet, but I have a need this weekend to try and implement forge-vite-typescript for a project that I need to present/propose on Monday (as a POC). Is it pretty much in the state that will be released in the next forge version? just needs the remaining tests to pass? In other words, can I go off this for now and assume it will be close to what is released with forge? To be clear, here is the repo I am referring to: https://github.com/caoxiemeihao/forge/tree/vite-typescript

@ronaldcurtis
Copy link
Contributor

After reproducing the error using @BlackHole1 's Dockerfile, it seems like the api.package line seems to be involved with the issue. Commenting out that test results in the subsequent WebpackTypeScript_spec_slow.ts to pass.

I guess some kind of side-effect is happening in api.package that needs to be cleaned up?

it('should pass', async () => {
await api.package({
dir,
interactive: false,
});
});

@BlackHole1
Copy link
Member

@GitMurf I can't guarantee that this PR will appear in the next release, but it looks like if it solves the test case problem, it will be merged soon.

In other words, can I go off this for now and assume it will be close to what is released with forge?

I think it should be no problem.

@ronaldcurtis good investigation 👍. cc @caoxiemeihao

@caoxiemeihao
Copy link
Member Author

caoxiemeihao commented Aug 13, 2023

Oh! I figured it out! -- CI failed on the Windows platform 😥
esbuild of used in the Vite can not exit normally on the Windows platform, now we can kill by manually, after I'll open an issue to Vite and esbuild repo explaining the problem.

cc @BlackHole1

* TODO: resolve `esbuild` can not exit normally on the Windows platform.
* @deprecated
*/
async function killWindowsEsbuildExe() {
Copy link
Member Author

Choose a reason for hiding this comment

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

@BlackHole1 Until Vite fix it, we can resolve the problem like this.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Is there an open issue tracking this for Vite?

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

It would be better if a Promise's resolve type is fixed and reject should always be passed with an error.

Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @caoxiemeihao!

import { defineConfig } from 'vite';

// https://vitejs.dev/config
export default defineConfig({});
Copy link
Member

Choose a reason for hiding this comment

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

Looks like Vite's default cache directory is node_modules/.vite, so we end up packaging that folder along with the project dependencies when building the project. Do you think it would be possible to use cacheDir: './.vite' as the default in the Vite configs, either here or maybe directly in the Vite plugin?

Copy link
Member Author

@caoxiemeihao caoxiemeihao Aug 15, 2023

Choose a reason for hiding this comment

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

The directory node_modules/.vite is esbuild Pre-Bundling cache dir, it's only work vite serve phase and ignored vite build phase by default. (@electron-forge/plugin-vite just run vite build)
Even if it is used in the vite build phase, it is rebuild by Vite into the dist folder.

Copy link
Member

Choose a reason for hiding this comment

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

The directory node_modules/.vite is esbuild Pre-Bundling cache dir

That's why we don't need it in node_modules when the app is packaged - the contents of this folder are not necessary for the app to run, but it's being included anyway 😅

In my case, I tested the template with a hello world React app (just electron-squirrel-startup, react and react-dom as dependencies), and my out/<project_name>/resources/app/node_modules/.vite folder was about 2.5 MB. No big deal, but I'm thinking it might become more of a problem for larger apps with multiple dependencies, so it might be worth addressing that somehow (maybe just deleting the folder from node_modules before packaging or using the packageAfterCopy hook could do the trick here).

That being said, I have zero experience with Vite and I don't how large this folder can get - if it never exceeds a couple megabytes, it might not be worth the hassle.

Copy link
Member

@BlackHole1 BlackHole1 Aug 16, 2023

Choose a reason for hiding this comment

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

I think we can discuss this further in another Issue / PR. If it’s really necessary and the implementation is not complicated, we can add a Good first issue tag to this particular issue.

This PR has been open for 5 months now, and if there are no obvious issues, my personal suggestion would be to proceed with the merge.

PTAL @erickzhao @dsanders11 @erikian

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me 👍

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me too. Let's fix both at the same time.

* TODO: resolve `esbuild` can not exit normally on the Windows platform.
* @deprecated
*/
async function killWindowsEsbuildExe() {
Copy link
Member

Choose a reason for hiding this comment

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

Is there an open issue tracking this for Vite?

import { defineConfig } from 'vite';

// https://vitejs.dev/config
export default defineConfig({});
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me too. Let's fix both at the same time.

@caoxiemeihao
Copy link
Member Author

Is there an open issue tracking this for Vite?

I'll be doing this over the weekend, my work environment only has a Mac to work with, it requires a Windows. :)

@erickzhao erickzhao merged commit f7018a9 into electron:main Aug 17, 2023
3 checks passed
@BlackHole1 BlackHole1 mentioned this pull request Sep 15, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.