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: angular integration #129

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat: angular integration #129

wants to merge 6 commits into from

Conversation

nartc
Copy link

@nartc nartc commented Jun 28, 2023

This PR adds neodrag integration with Angular

  • NeoDraggable directive
  • Documentations updated with Angular

Would love some help to set up changesets for publishing.

Closes #106

@changeset-bot
Copy link

changeset-bot bot commented Jun 28, 2023

⚠️ No Changeset found

Latest commit: 8bf5d7f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
neodrag ❌ Failed (Inspect) Dec 20, 2023 8:06am

@vercel
Copy link

vercel bot commented Jun 28, 2023

@nartc is attempting to deploy a commit to the Purus Projects Team on Vercel.

A member of the Team first needs to authorize it.

@nartc nartc mentioned this pull request Jun 28, 2023
@nartc nartc force-pushed the feat/ng branch 4 times, most recently from d87d441 to 724193e Compare June 28, 2023 20:55
@trungvose
Copy link

trungvose commented Jun 29, 2023

Here are the steps to run the test locally and the final result:

image

  1. Start by running pnpm install on the root directory. Make sure you are using pnpm version 8. ℹ️ Note: Initially, I encountered an issue because I was using pnpm version 7. Here is a screenshot of the error:

image

  1. After installing the dependencies, still on the root directory, run npx nx compile @neodrag/angular. If everything is successful, you should see a similar output as shown in the screenshot.

image

  1. Run pnpm install again to ensure that @neodrag/angular is installed in the demo project.

  2. Now navigate to the packages/angular/demo directory and run npm start. There are two possible scenarios that can occur, and both @nartc and I are investigating the cause.

    4.1 ✅ Everything works fine, and you should see the expected output as shown in my screenshot.

    4.2 ❌ The Angular dev server runs successfully, but you encounter an error in the browser console. To resolve this issue, go back to the root folder, delete the node_modules and pnpm-lock files, and then run pnpm install again. For some reason, when running pnpm install, rxjs@7.8.1 gets installed, but if we remove the pnpm-lock file, rxjs@7.8.0 is installed instead.

Monosnap main ts — neodrag 2023-06-29 14-53-37

I hope this helps! Let me know if you have any further questions.

@nartc
Copy link
Author

nartc commented Jun 30, 2023

@PuruVJ is there a strategy you have in mind for including the angular integration now in terms of release version/changeset?

@PuruVJ
Copy link
Owner

PuruVJ commented Jul 1, 2023

It's a bit tricky. it should go out as 0.x release first. Then as it gains more attractions within a month, releasing 2.0.0 feels fine. I know skipping over 1.0.0 is weird, but I dont think angular users would mind, considering the whole thing with skipping angular 3 and updating other deps directly v4 as well.

I haven't given this much time yet. It seems the docs site is also breaking due to old version of something and that'll need fixing before this can be merged. I'm on a bit of a limited bandwidth for next few days due to some urgent bugs in svelte REPL and sites. Once that's done, I'll give this PR a full review along with updated docs site.

@sheikalthaf
Copy link

@PuruVJ can you please look into this PR and merge it. I'm waiting for this support

@PuruVJ
Copy link
Owner

PuruVJ commented Dec 18, 2023

Hi! I am able to work on this finally!!

Thanks a lot for this @nartc. Ill be going through the changes and asking questions to understand it. However, I dont really understand angular deeply, so I'll be going forward with the assumption that you took care of the edge cases. I have full confidence in you, considering your background and experience 🔥😁

Once again, thanks a lot for this!

@PuruVJ
Copy link
Owner

PuruVJ commented Dec 18, 2023

I have one question: Is ng-packagr necessary? Can't we compile the decorators down to their plain JS version with tsup and ship it?

@nartc
Copy link
Author

nartc commented Dec 18, 2023

I'll check it out later today. Thanks @PuruVJ for taking a look.

@nartc
Copy link
Author

nartc commented Dec 19, 2023

@PuruVJ I've updated the PR. There are some changes that I want to point out:

  • Can we go with TypeScript 5.2 at the moment? The Angular Compiler has yet to support 5.3 (it should be soon). To answer your question about ng-packagr, it is required because Angular decorators aren't just TypeScript decorators. Those decorators will be compiled away using the Angular Compiler into Angular Instructions (template instructions)
  • Angular Library is not minified as the Angular Team encourages folks to only minify at the application level (where the library is consumed). This makes the Angular size look huge on the docs
image

@PuruVJ
Copy link
Owner

PuruVJ commented Dec 20, 2023

Re: minification - Other versions also ship unminified version, but the sizes are calculated from dist/min/index.js, which is minified, but isn't used directly, unless someone explicitly uses its path.

Is it possible to have that for Angular integration too? Just for reporting the size?

@nartc
Copy link
Author

nartc commented Dec 20, 2023

@PuruVJ, It is possible to run a script to minify Angular's output, but it wouldn't seem fair either since the other min/index.js seem to have @neodrag/core bundled while the Angular one does not bundle its dependencies.
image

@PuruVJ
Copy link
Owner

PuruVJ commented Dec 20, 2023

Wait what?! That doesn't seem right. @neodrag/core's functionality should be bundled inside the package. Is that possible?

@nartc
Copy link
Author

nartc commented Dec 20, 2023

It shouldn't since Angular bundles external packages and optimizes them for end users. Libraries' dependencies are either implicit deps (e:g: dependencies) or explicit peer deps. All Angular libs are built this way. Angular Team ensures libs built with Angular Package Format to work properly for end users.

@PuruVJ
Copy link
Owner

PuruVJ commented Dec 20, 2023

Aha!

Is it possible to make a separate minified file with decorators intact, but entirety of @neodrag/core included, and then minified? I'd still like the users to get an idea of how big this is gonna be

@nartc
Copy link
Author

nartc commented Dec 20, 2023

I can figure something out

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.

Angular support
4 participants