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

simplify setup #4

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

Conversation

sudo-barun
Copy link

  • CommonJS module is no longer generated
  • generated js code and ts file stays at same directory and looks almost similar

- CommonJS module is no longer generated
- generated js code and ts file stays at same directory and looks almost similar
@bramus
Copy link
Owner

bramus commented Aug 31, 2024

While I certainly agree that the build system could use an update – it is a 4 year old build system that was part of the original https://github.com/fluorumlabs/css-variable-observer project – and agree we should get rid of the CommonJS module, I don’t entirely agree with the contents of the PR.

  • The contained index.js is the output of a TypeScript compilation. That file should not be part of the repo but only of the published module.
  • The entire using-typescript example is overhead.
  • The import in the original example should remain importing from @bramus/style-observer

If could stick to the core, I’ll happily accept a PR that updates the build system/setup.

@sudo-barun
Copy link
Author

Thanks for having a look at the PR. I will make necessary changes later.

  • I will remove the the index.js.
    However, please reconsider on including the js file as well. Having js file has some benefits. means that someone cloning this repo don't have to install TypeScript and other devDependencies just to run the demo. Also, someone who is unfamiliar with TypeScript or someone who just wants to look at the code that actually runs in the browser can looks the js file from GitHub.

  • I will revert import in example to use @bramus/style-observer by making use of importmap. I hope that's fine? (I am asking this since some people don't like to have node_modules in import or importmap.)

Also, this PR can cause breaking changes if released as minor version. Are you planning to release v2? 😁

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.

2 participants