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(qwikVite): add possibility to define multiple outputs #6452

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

KingSora
Copy link
Contributor

@KingSora KingSora commented Jun 3, 2024

Overview

What is it?

  • Feature / enhancement
  • Bug
  • Docs / tests / types / typos

Description

I'm using qwikVite to bundle my library. Because my setup requires multiple output files per format, I've tried to use the rollupOptions.output field to achieve this:

{
  build: {
    lib: {
      entry: './src/index.ts',
    },
    rollupOptions: {
      output: [
        {
          format: 'es',
          entryFileNames: 'index.esm.js',
        },
        {
          format: 'es',
          entryFileNames: 'index.mjs',
        },
        {
          format: 'cjs',
          entryFileNames: 'index.cjs.js',
        },
        {
          format: 'cjs',
          entryFileNames: 'index.cjs',
        },
      ],
    },
  }
}

This did work but always created one additional output file which I didn't specify in the config. That happens because the qwikVite plugin is not handling output arrays correctly.

Use cases and why

The possibility to achieve multiple outputs for library authors.

Checklist:

  • My code follows the developer guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • Added new tests to cover the fix / functionality

@KingSora KingSora requested a review from a team as a code owner June 3, 2024 15:51
Copy link

netlify bot commented Jun 3, 2024

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 7c6eea1

@KingSora KingSora changed the title [qwikVite] add possibility to define multiple outputs feat(qwikVite): add possibility to define multiple outputs Jun 3, 2024
@PatrickJS
Copy link
Member

amazing 🙏

PatrickJS
PatrickJS previously approved these changes Jun 3, 2024
@KingSora
Copy link
Contributor Author

KingSora commented Jun 3, 2024

Thanks for the approve @PatrickJS, I've added one small improvement in case the provided output array is empty

Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

Some small changes, looks good overall!

packages/qwik/src/optimizer/src/plugins/rollup.ts Outdated Show resolved Hide resolved
packages/qwik/src/optimizer/src/plugins/rollup.ts Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Jun 14, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

commit: 7c6eea1

@builder.io/qwik

npm i https://pkg.pr.new/@builder.io/qwik@6452

@builder.io/qwik-city

npm i https://pkg.pr.new/@builder.io/qwik-city@6452

eslint-plugin-qwik

npm i https://pkg.pr.new/eslint-plugin-qwik@6452

create-qwik

npm i https://pkg.pr.new/create-qwik@6452

wmertens
wmertens previously approved these changes Jun 14, 2024
Copy link
Member

@wmertens wmertens left a comment

Choose a reason for hiding this comment

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

LGTM

@wmertens wmertens enabled auto-merge (squash) June 14, 2024 08:14
@wmertens wmertens merged commit 97dbcaa into QwikDev:main Jun 14, 2024
20 checks passed
genki pushed a commit to genki/qwik that referenced this pull request Jun 28, 2024
)

* add possibility to define multiple outputs in vite

* more explicit assertions in tests

* simplify code

* small improvement to make sure at least one output is present in every case

* change variable name so the history is easier to follow

* fix(vite.unit.ts): grab plugin

---------

Co-authored-by: PatrickJS <github@gdi2290.com>
@wmertens wmertens mentioned this pull request Jul 9, 2024
7 tasks
@diecodev
Copy link
Contributor

Hey @KingSora @PatrickJS @wmertens after the merge of this PR, the vite config is overwrited.

  • this is my vite.config.ts file:
import { defineConfig } from "vite";
import pkg from "./package.json";
import { qwikVite } from "@builder.io/qwik/optimizer";
import tsconfigPaths from "vite-tsconfig-paths";

const { dependencies = {}, peerDependencies = {} } = pkg as any;
const makeRegex = (dep) => new RegExp(`^${dep}(/.*)?$`);
const excludeAll = (obj) => Object.keys(obj).map(makeRegex);

export default defineConfig(({ command }) => {
  return {
    plugins: [qwikVite(), tsconfigPaths()],
    build: {
      target: "es2020",
      lib: {
        entry: "src/lib/index.ts",
        formats: ["es", "cjs"],
        fileName: (format, entry) => {
          console.log({ format, entry });
          const ext = format === "es" ? "mjs" : "cjs";
          const name = entry;
          return `${name}.qwik.${ext}`;
        },
      },
      rollupOptions: {
        // externalize deps that shouldn't be bundled into the library
        external: [
          /^node:.*/,
          ...excludeAll(dependencies),
          ...excludeAll(peerDependencies),
        ],
      },
    },
  };
});

as you can see, I do not have a rollup custom configuration, and also, I got setup the build.lib.formats in the config, but take a look to what the console.log says when I try to run the build command:

image
  • this is my pkg json:
image

Note

I tried with qwik@1.5.7 and everything works as expected (I got 2 builded file ~ .mjs and .cjs) after update to qwik@1.6.0 and I got the build error (only a .mjs) file

@wmertens
Copy link
Member

Ok we have to revert this, I also encountered it without realizing what it was.

@KingSora can you explain why you needed this and why you can't use the vite lib option?

@KingSora
Copy link
Contributor Author

@wmertens All of the behavior changes are happening because of my assumption of this:

rollupOptions: {
   output: [
     {
       /** ...my output options... */
     }
   ]
}

being equivalent to this:

rollupOptions: {
   output: {
     /** ...my output options... */
   }
}

I can submit a PR if you are interested to fix this behavior with the suggestion I did in #6643:

The qwikVite plugin doesn't always return an array as output.. e.g. if the output field is an object the plugin won't transform it into an array with a single entry (as it is happening now)

This should restore the original behavior for all users but keep the possibility to define multiple outputs if you need them.

can you explain why you needed this and why you can't use the vite lib option?

Yes, for a library I'm working on I needed multiple build targets an a way that vites formats is not supporting (e.g. two or more outputs for one format)

@wmertens
Copy link
Member

@KingSora ok, I propose you add a helper function in plugin.ts that can extract the first input from any form of input, and that you normalize the inputs without changing their form. So then the image plugin stuff etc will still break for multi output but at least not in normal use and hopefully that gets fixed sometime.

@KingSora
Copy link
Contributor Author

@wmertens I've opened a PR #6723 to revert the behavior for cases where output is an object but also support cases where output is an array of objects

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.

4 participants