-
Notifications
You must be signed in to change notification settings - Fork 922
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
Simplify. cleanup, enhance snowpack internals #2707
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/GoQNJ6Svi24aXAV2AoxFYrWYF6WD |
2c21a9b
to
2803d2f
Compare
2803d2f
to
dbbecb1
Compare
dbbecb1
to
1e446eb
Compare
1e446eb
to
2929f8d
Compare
2929f8d
to
eae41c6
Compare
eae41c6
to
89ecca5
Compare
89ecca5
to
245d205
Compare
245d205
to
626906f
Compare
626906f
to
3f3b22a
Compare
3f3b22a
to
9a1ab0b
Compare
9a1ab0b
to
92b5659
Compare
f48f7c4
to
177c3a7
Compare
Found a strange error while testing this:
/Users/megatron/.npm/_logs/2021-03-14T01_13_50_661Z-debug.log:
I've pushed the very simple demo project here in case anyone would like to repro: https://github.com/joehenry087/snowpack-test Follow the readme instructions and then just change the contents of the "Sub" module and it will trigger this issue. I've messed with the snowpack config and verified my node versions and all that... kind of out of options at this point. Would be interested to know if this is just a problem with my setup or with the new HMR work. Also wanted to say thank you for putting together the new HMR watch stuff @FredKSchott |
Ah, it seems if there is a svelte file that does not produce a stylesheet it will break. This may be an issue with the Svelte plugin rather than snowpack? I'll keep digging. Just adding some CSS to the svelte modules is a workaround:
|
@joehenry087 thanks for posting! I had thought that we found this bug originally and fixed it, will confirm its not a regression. |
strange, I can't reproduce in your repo ( |
@FredKSchott just to confirm, you must then edit the If you did that and it works, it must be a problem with my node or npm or something... |
It is odd that running |
I am having trouble finding any reference to a previous bug like this. |
I can reproduce! Didn't realize it was only on save, but that made it easy to narrow down. Fixed in e9dfbac, will go out in the next |
Brilliant! So glad this was an actual bug, usually it's just me derping on an incorrect configuration 😂 |
@FredKSchott regarding this:
I have no issues building the appropriate client/server bundles for production (using
The local components work, but none of the components from
Any thoughts? Has this been tested? I am working from a Lerna mono-repo and setup
In addition, when running my app in non-ssr mode (but using the DevServer inside an Express app), I'm getting this error for a component that is linked within the mono-repo:
Lastly, the HMR websock at port 3002 (which was setup correctly) doesn't seem to respond since updating to
|
@FredKSchott - trying the new release [3.1.0-pre13], as an improved support for linked dependencies is very much welcome (Thank You !). So far with little success. My app was able to run under 3.0, with a couple of hacks to circumvent linked repositories however (see #2700). The dev server is unable to find some modules, as I get quite a lot of errors like:
Also, it seems that mounted routes are not working anymore when they contain symlinks. This config is not working anymore, as if __ is not mounted:
<script src="/__/firebase/8.2.9/firebase-app.js"></script> |
Huh, that's odd. For some reason Snowpack is treating your Definitely not expected, at the very least we could explore adding an error to the source file builder if it looks like its trying to build a dependency. Also if |
@FredKSchott - thanks a lot I am trying (hard!) to understand what might be happening, and could reproduce on a simplified test app, by reintroducing dependencies one by one setup, using pnpm: test app:dependencies": {
"@material/base": "^10.0.0",
"@material/mwc-tab": "^0.20.0",
"@preignition/preignition-demo": "link:../litElement/preignition-demo",
"@preignition/preignition-widget": "link:../litElement/preignition-widget",
"lit-element": "^2.4.0",
"lit-html": "^1.3.0",
"router-slot": "^1.5.4"
}, preignition-widget - some dependencies are also symlinked: "dependencies": {
"@material/... a couple of them ...": "^0.20.0",
"@preignition/lit-firebase": "link:../lit-firebase",
"@preignition/preignition-mixin": "link:../preignition-mixin",
"@preignition/preignition-state": "link:../preignition-state",
"@preignition/preignition-styles": "link:../preignition-styles",
"@preignition/preignition-util": "link:../preignition-util",
"lit-element": "^2.4.0",
"lit-html": "^1.3.0",
"tslib": "^2.0.0"
}, I start to see the 'MODULE_NOT_FOUND' error when I import some For instance, if this file importing import { Select } from '@material/mwc-select';
import TwoWaySelectBinding from './two-way-select-binding.js';
/**
* extension of mwc-textfield emiting a value-changed event when
* value changes
*/
class PwiSelect extends
TwoWaySelectBinding(
Select)))) {
}
}
customElements.define('pwi-select', PwiSelect);
export { PwiSelect }; will generate :
If I remove this file or remove from the export or remove the Interestingly deduping some dependencies in the test-app ( This workaround did not work in other instances. For instance, importing
|
I am also observing some weird stuff re lit-element directives. First time I run the app (with
Hard refreshing on the browser makes the app renders fined. Also sometimes the Edit: seems related to #2461 |
Hey @FredKSchott had some time to test this lately and I want to say that it works pretty good, the only downside (that I think that it may be introduced with those changes) is yup not working (looks like the file is not generated correctly), there's a ticket here: #2945 but just wanted to link both tickets since seems that downgrading solves (except if you run a monorepo 😂 ) |
Hey @FredKSchott did you get a change to checkout the repro for this regression? Wanted to followup since someone else is reporting the same but I haven't tested out the latest and greatest |
} | ||
} | ||
const isHMR = getIsHmrEnabled(config); | ||
config.buildOptions.resolveProxyImports = !config.optimize?.bundle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems to break image import for bundled builds.
[BUG] sowpack bundle build generate import statement for images and json files
If we turn on optimize.bundle, resolveProxyImports is always going to be false, cause the js bundle to import image files directly:
import logo from './logo.svg''
This can be reproduced with @snowpack/app-template-react by turning on optimized.bundle.
Changes
snowpack dev
- 500 lines of code refactored away, old branching code paths now unified into a single pipeline.snowpack build
- ~350 lines of code refactored away, including a full rewrite of the build pipeline to reuse the dev server internally for file builds.isPackage
flag had been added to the pluginload()
hook so that plugins can customize their behavior.Issues resolved
Testing
Docs