-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix journey manager umd #127
Conversation
@@ -65,9 +65,7 @@ export default function buildconfig({ | |||
commonjs(), // needed to bundle node modules that use cjs | |||
json(), // needed to fetch package version from package.json | |||
nodeResolve({ browser: true }), // bundles packages from node_modules | |||
nodePolyfills({ | |||
include: "../../node_modules/**/*.js", |
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.
before #119 this value was "../**/node_modules/**/*.js"
(presumably taken from ionic-team/rollup-plugin-node-polyfills#17 (comment)).
In 119 I changed it to ../..
to point to the proper node_modules
path which may have caused this regression.
I still don't understand why ../**/node_modules
is relevant. I believe in the comment linked above, it's for a different monorepo setup where node_modules
are symlinked into package directories.
In any case, the change to ../../
broke things. I tried to avoid regressions like this by diffing umd output before and after #119 changes, but apparently that testing was insufficient!
Do you understand what the previous include
param was doing? If not I'd rather drop it altogether (like I do in this PR)
used this html file to test the results of this change In no way a comprehensive test! |
I haven't tested this extensively (across other packagers, for instance) but it appears to at least work for journey manager