Support packageOptions.rollup.plugins #3123

haruhihi · 2021-04-09T07:52:51Z

Changes

Testing

Docs

vercel · 2021-04-09T07:52:55Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/EuMTZTnRsYRVVtKExR1bGnh1oBVg
✅ Preview: https://snowpack-git-fork-haruhihi-fix-packageoptions-rollup-pikapkg.vercel.app

FredKSchott · 2021-04-10T04:22:56Z

Just curious: what are you using this for?

Starting in Snowpack v3.2, we send all dependencies through your normal source build pipeline. We were only aware of rollup plugins being used to get around that limitation, back when dependencies weren't sent through your normal build pipeline.

haruhihi · 2021-04-10T04:50:22Z

Just curious: what are you using this for?

Starting in Snowpack v3.2, we send all dependencies through your normal source build pipeline. We were only aware of rollup plugins being used to get around that limitation, back when dependencies weren't sent through your normal build pipeline.

@FredKSchott After I upgrade to 3.1.2, the scss files in node_modules fails in build #2970, and the same problems like #2999. I found the packageOptions.rollup.plugins didn't work any more. In 3.0.11 I use packageOptions.rollup.plugins to deal with the scss files in node_modules, and it works well. So I create this PR.

Did you mean I should add those plugins to the plugins in snowpack.config.json ? I will try this later

Thanks for you great work, I love snowpack 👍

fgblomqvist · 2021-04-13T15:18:57Z

@FredKSchott I didn't see anything about sending all deps through the normal build pipeline in the changelog for 3.2, did you mean a different version or did it simply sneak out of the changelog? 🙃

haruhihi · 2021-04-15T08:27:48Z

Just curious: what are you using this for?
Starting in Snowpack v3.2, we send all dependencies through your normal source build pipeline. We were only aware of rollup plugins being used to get around that limitation, back when dependencies weren't sent through your normal build pipeline.

@FredKSchott After I upgrade to 3.1.2, the scss files in node_modules fails in build #2970, and the same problems like #2999. I found the packageOptions.rollup.plugins didn't work any more. In 3.0.11 I use packageOptions.rollup.plugins to deal with the scss files in node_modules, and it works well. So I create this PR.

Did you mean I should add those plugins to the plugins in snowpack.config.json ? I will try this later

Thanks for you great work, I love snowpack 👍

But It doesn't work : (

drwpow · 2021-04-19T20:53:37Z

@haruhihi Sorry for the unexpected change. Which npm packages are you having issues with? And are you able to provide a reproduction of the issue?

As Fred said, the rollup.plugins config option has always been a bit of a bandaid that can cause a worse experience for some people. And long-term, we may not even keep using Rollup! So I’d like to see if we can make Snowpack plugins work for you for your existing setup, and hopefully it’s a better fix long-term.

drwpow · 2021-04-19T20:55:37Z

@FredKSchott I didn't see anything about sending all deps through the normal build pipeline in the changelog for 3.2, did you mean a different version or did it simply sneak out of the changelog? 🙃

I believe this happened under #2707 as part of v3.1.0 actually.

haruhihi · 2021-04-20T06:19:25Z

@haruhihi Sorry for the unexpected change. Which npm packages are you having issues with? And are you able to provide a reproduction of the issue?

As Fred said, the rollup.plugins config option has always been a bit of a bandaid that can cause a worse experience for some people. And long-term, we may not even keep using Rollup! So I’d like to see if we can make Snowpack plugins work for you for your existing setup, and hopefully it’s a better fix long-term.

@drwpow The issue here #2970 #2999. The npm package is belong to my company, I can't put it here. I will try to find an open source package to reproduce later. Seems the plugin for .scss, .svg doesn't work on node_modules.

eytienne

could maybe use deepmerge as in config.ts

eytienne · 2021-04-29T12:52:43Z

snowpack/src/sources/local-install.ts

@@ -52,6 +52,7 @@ export async function installPackages({
    stats: false,
    rollup: {
      plugins: [
+        ...(installOptions?.rollup?.plugins ?? []),


first ?. is useless

eytienne · 2021-04-29T12:59:57Z

snowpack/src/sources/local.ts

@@ -575,6 +575,9 @@ export class PackageSourceLocal implements PackageSource {
          if (config.packageOptions.namedExports !== undefined) {
            installOptions.namedExports = config.packageOptions.namedExports;
          }
+          if (config.packageOptions.rollup !== undefined) {
+            installOptions.rollup = config.packageOptions.rollup;
+          }


the root cause of this bug seems to be that confusing redundancy between call site and function body

eytienne · 2021-04-29T16:10:56Z

@FredKSchott @drwpow this bug seems critical.. or maybe I missed something.. it blocks me trying to rewrite css urls via a plugin of a rollup plugin (rollup-plugin-postcss)
I could do a PR around that area whose some pieces confuse me or hurt my reading. Is there any docs for contributors except the root .md? It would be a good idea to introduce the global vision and implementation.

drwpow · 2021-04-29T18:23:11Z

@FredKSchott @drwpow this bug seems critical.. or maybe I missed something.. it blocks me trying to rewrite css urls via a plugin of a rollup plugin (rollup-plugin-postcss)
I could do a PR around that area whose some pieces confuse me or hurt my reading. Is there any docs for contributors except the root .md? It would be a good idea to introduce the global vision and implementation.

Yeah that’s fair. I wanted to believe that the correct fix was to run Snowpack plugins over all node_modules, and deprecate this option. But after reflecting on it, I’m happy to merge it, because in all cases I think removing Rollup plugins for now would be a breaking change. It’s still in our docs, after all.

drwpow · 2021-04-29T18:52:45Z

Published as snowpack@next (3.3.6-pre.0). @eytienne @haruhihi could you test this in your local projects and let me know if everything works as expected? If so, I can release the patch; if not, we can merge another PR if necessary before patching.

eytienne · 2021-04-29T18:58:13Z

I'll do. Actually, I thinked it was the default behaviour to apply plugins on all files.

haruhihi · 2021-04-30T09:07:07Z

@drwpow @eytienne Thanks, After upgrade I see the plugins works :) . But I get another problem now. The jspdf package is broken.
Here is the error

[17:49:13] [snowpack] Cannot find module ''./common/html2canvas.esm-da61cd89.js'' from '[my project]/node_modules/jspdf/dist/jspdf.es.min.js'

I see there are four single quotes around ./common/html2canvas.esm-da61cd89.js, that's strange

eytienne · 2021-04-30T09:25:16Z

@drwpow @haruhihi Yes it works, I just have to make my particular plugin work now. I bring to your attention this feedback: #3240

fix: support packageOptions.rollup.plugins

f8b1754

vercel bot deployed to Preview April 9, 2021 07:52 View deployment

Update local.ts

fc043c0

vercel bot deployed to Preview April 9, 2021 09:40 View deployment

Update local-install.ts

1a8447d

vercel bot deployed to Preview April 9, 2021 09:44 View deployment

eytienne suggested changes Apr 29, 2021

View reviewed changes

drwpow merged commit 3701201 into FredKSchott:main Apr 29, 2021

harleylang mentioned this pull request May 11, 2021

[BUG] [3.1.2] Fail to esinstall node_modules files with .svg extensions #2999

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support packageOptions.rollup.plugins #3123

Support packageOptions.rollup.plugins #3123

haruhihi commented Apr 9, 2021

vercel bot commented Apr 9, 2021 •

edited

FredKSchott commented Apr 10, 2021

haruhihi commented Apr 10, 2021 •

edited

fgblomqvist commented Apr 13, 2021

haruhihi commented Apr 15, 2021

drwpow commented Apr 19, 2021

drwpow commented Apr 19, 2021

haruhihi commented Apr 20, 2021

eytienne left a comment

eytienne Apr 29, 2021

eytienne Apr 29, 2021 •

edited

eytienne commented Apr 29, 2021

drwpow commented Apr 29, 2021

drwpow commented Apr 29, 2021

eytienne commented Apr 29, 2021

haruhihi commented Apr 30, 2021

eytienne commented Apr 30, 2021

Support packageOptions.rollup.plugins #3123

Support packageOptions.rollup.plugins #3123

Conversation

haruhihi commented Apr 9, 2021

Changes

Testing

Docs

vercel bot commented Apr 9, 2021 • edited

FredKSchott commented Apr 10, 2021

haruhihi commented Apr 10, 2021 • edited

fgblomqvist commented Apr 13, 2021

haruhihi commented Apr 15, 2021

drwpow commented Apr 19, 2021

drwpow commented Apr 19, 2021

haruhihi commented Apr 20, 2021

eytienne left a comment

Choose a reason for hiding this comment

eytienne Apr 29, 2021

Choose a reason for hiding this comment

eytienne Apr 29, 2021 • edited

Choose a reason for hiding this comment

eytienne commented Apr 29, 2021

drwpow commented Apr 29, 2021

drwpow commented Apr 29, 2021

eytienne commented Apr 29, 2021

haruhihi commented Apr 30, 2021

eytienne commented Apr 30, 2021

vercel bot commented Apr 9, 2021 •

edited

haruhihi commented Apr 10, 2021 •

edited

eytienne Apr 29, 2021 •

edited