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

Import phoenix js dependencies directly from /deps #444

Closed
wants to merge 2 commits into from

Conversation

mveytsman
Copy link
Contributor

This avoids the problems that we have been having with importing our phoenix js dependencies as filesystem NPM packages, for example see phoenixframework/phoenix_live_view#1369 (comment) as a PR we had to revert because of this pattern.

Here we import the js from phoenix, phoenix_html, and phoenix_live_view directly from the built.js file in the deps folder, instead of importing an npm package from the same folder.

We use source-map-loader to provide sourcemaps in development. The source map for phoenix.js is built and included, the map for phoenix_live_view.js will be available when phoenixframework/phoenix_live_view#1502 ships

If this is 👍 I will add a similar PR to the phoenix generators to do this for new projects.

@CLAassistant
Copy link

CLAassistant commented Jul 9, 2021

CLA assistant check
All committers have signed the CLA.

@jonatanklosko
Copy link
Member

Hey @mveytsman! Currently LV package.json points only to the built file. There was/is indeed an issue with having "exports" in package.json, because then Webpack uses it for loading the package instead, and because of bug in npm7 LV dependencies (morphdom) are not resolved correctly. However, the "exports" property has been recently reverted, so the current setup works fine, please correct me if I miss anything :)

@mveytsman
Copy link
Contributor Author

@jonatanklosko that's correct! My thinking is this change will let us avoid this or similar bugs in the future. Basically the issue is that we have to track our dependencies on LV in both mix.exs and in package.json, which has caused a lot of issues for the Phoenix team. With this change we are only tracking dependencies in mix.exs.

The thinking is this should be how we structure dependencies for all Phoenix/LV projects in the future.

I chose to make this PR first against liveview because it had the npm@7 issues that caused @josevalim to revert that exports property (I think the direct dependency on morphdom had something to do with it). If this is agreed to be the correct path I will make the same PR for the generators and the other projects we're supporting (e.g. LiveDashboard).

@jonatanklosko
Copy link
Member

Hmm, if the top-level package.json points only to the built assets via "main", then I think both approaches are pretty much equivalent. And if the goal is to make "exports" work, then I don't think this helps, because the dependencies (if any) won't be installed. What's the advantage here?

@mveytsman
Copy link
Contributor Author

Closing this as it's the wrong place for this work and I'm working on emitting ESM for phoenix.js and phoenix_live_view.js in the correct projects. Sorry to pollute your issues/PRs @jonatanklosko ❤️ ❤️ ❤️

@mveytsman mveytsman closed this Jul 15, 2021
@mveytsman mveytsman deleted the importfiles branch July 15, 2021 21:44
@jonatanklosko
Copy link
Member

@mveytsman no worries, thanks for investigating!

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.

3 participants