aurelia / ux

A user experience framework with higher-level capabilities, designed to bring simplicity and elegance to building cross-device, rich experiences.
MIT License
368 stars 55 forks source link

fix(all): separate dev and build tsconfigs #264

Closed MaximBalaganskiy closed 4 years ago

MaximBalaganskiy commented 4 years ago

This allows for removing dist files from the repository. VS code picks up definitions straight from src folders. The package tsconfigs just reference the higher level ones.

In addition, packages now explicitly import templates which removes the need to use ModuleDependenciesPlugin.

bigopon commented 4 years ago

This seems beneficial to development. Though we may have issues with folks who rely on dist on master branch

cc @fkleuver @EisenbergEffect

3cp commented 4 years ago

Generally, dist files should be ignored in git repo.

If user uses git repo as the dependency in their package.json, the missing dist files is not a problem, as long as this repo's package.json has proper npm script "prepare": "npm run build". (The changes in this PR is too big for me to confirm whether there is a "prepare" script.)

When npm install a git repo dep (not a normal npm dep) which has a "prepare" script, it will run it (install dev deps before running it too) to build dist files in our case.

But yarn and pnpm did not cover this npm behaviour, mainly due to lack of understanding when they were first designed. https://github.com/yarnpkg/yarn/issues/5235 https://github.com/pnpm/pnpm/issues/855

BTW, yarn team ignored my fix. https://github.com/yarnpkg/yarn/pull/6131

MaximBalaganskiy commented 4 years ago

dist files are built before publish, so npm packages will have them. Intra repo development will use aliases to src folder. It is arguable if one should develop an external package and reference dist file from another dev folder.

fkleuver commented 4 years ago

Though we may have issues with folks who rely on dist on master branch

That's the thing. I've had to completely revert a bunch of CI work because of this before and this is the reason we have this convoluted develop->master merge and merge-back process in some of the au1 repos.

Getting rid of dist files on master is a breaking change for some.

It's not a breaking change that I am against, though, because the benefits would be significant for our development efforts, but I don't know how many people rely on this and how hard a sell it would be for them.

MaximBalaganskiy commented 4 years ago

It would be interesting to see a scenario where dist in master is needed.

fkleuver commented 4 years ago

It would be interesting to see a scenario where dist in master is needed.

Several years ago it was fairly common to install dependencies into the package.json or jspm config as a github url. It's less common now that npm has matured more, but apparently a not insignificant number of legacy projects still rely on this.

fkleuver commented 4 years ago

Again, I'm not against it per se, but we cannot just wipe it under the rug and pretend it's not a breaking change, because it is. And if we make this breaking change, we ought to do it for all Aurelia repos so we can simplify everything in the same way.

3cp commented 4 years ago

install dependencies into the package.json or jspm config as a github url

I mentioned "prepare" script in package.json can support that use case. Npm will build dist files during installation.

Removing dist files, add "prepare" script. It's only a breaking change for yarn/pnpm users. Not a breaking change for npm users.

fkleuver commented 4 years ago

It's only a breaking change for yarn/pnpm users.

And JSPM users, which I believe is the majority of people who use it this way. .. a breaking change for some, is still a breaking change

3cp commented 4 years ago

Right, jspm users (I totally ignored them) :)

MaximBalaganskiy commented 4 years ago

So, as an alternative, accept the tsconfigs, keep the dist, and somehow setup a hook to reset dist upon commit. Should be possible...

MaximBalaganskiy commented 4 years ago

Ok. The following hook will void any changes to dist before commit. Either add it to your .git/hooks folder or remember to do it manually

pre-commit

#!/bin/sh

git reset packages/*/dist/*.* # reset staged
git checkout packages/*/dist/*.* # revert changes
git clean -f packages/*/dist/*.* # clean new files
git clean -fd packages/*/dist/* # clean new folders
MaximBalaganskiy commented 4 years ago

Does this look ok now?

bigopon commented 4 years ago

@MaximBalaganskiy thanks for the PR 👍

ben-girardet commented 4 years ago

Looking forward to try this 👍🏻