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

Phase out dist/ folder in favour of qunit/ #1133

Closed
platinumazure opened this issue Mar 30, 2017 · 5 comments · Fixed by #1541
Closed

Phase out dist/ folder in favour of qunit/ #1133

platinumazure opened this issue Mar 30, 2017 · 5 comments · Fixed by #1541
Labels
Category: Release help welcome Status: Ready A "Meta" type issue that has reached consensus.
Milestone

Comments

@platinumazure
Copy link
Contributor

I'm a bit confused about why we maintain both a dist/ folder (for web use and development) but also create a qunit/ folder when releasing and/or for Node. Can we simplify this process?

Long-term, my preference would be to just export a canonical "core QUnit" file and also have wrappers for executing QUnit in different contexts. (In a different issue, I might have referred to those wrappers as "runners", to contract with "reporters".) But on top of that, it would be nice if we could keep the folder structure consistent if at all possible.

@leobalter
Copy link
Member

leobalter commented Mar 30, 2017

cc @jzaefferer. You might know the reason for this. I honestly don't remember.

@leobalter
Copy link
Member

@platinumazure This might become the new turtle on the tree. It's there by some reason.

@Krinkle
Copy link
Member

Krinkle commented Mar 31, 2017

@platinumazure At the moment, neither dist/ nor qunit/ is actually in the master branch. But they do spring into existence during local development and/or as part of the release process.

Release

Long ago (before 1.13.0, and even longer ago when it was still part of jquery's test directory) QUnit was maintained as a single file in the qunit/ directory.

I first moved away from this in 6f8f6c3 (pull #480, issue #378).

Since then, we've no longer had a qunit/ directory in master. However there were two uses cases relying on a qunit/ directory:

  • Use of QUnit through Git submodules. (Mostly older pre-npm projects.)
  • Use of QUnit through Bower. (Which required files to be in Git.)

It was proposed by @jzaefferer in #480 (comment) that we keep supporting this by committing the qunit/ build artefact into Git as part of the release process. That way it won't generally exist in Git, but we can put the release tag on a leaf-node commit (or "detached commit") outside the master branch lineage so that Bower and Git-submodule usage can still reference qunit.js without a post-install build process of any sort.

We were going to only keep doing that for new releases until 2.0, but I guess we forgot!

Development

During local development, grunt will generate a distribution file in dist/, which is used when loading the test suite in a web browser. This is a different directory name to avoid conflicts when working on releases, and in general since they use slightly different logic to be generated (one containing a git hash, the other a version tag).

As far as I know, dist/ is never committed to Git.

@Krinkle Krinkle added Type: Meta Seek input from maintainers and contributors. Category: Release labels Mar 31, 2017
@trentmwillis
Copy link
Member

@Krinkle thanks for the background, much appreciated!

At this point, since we ship qunit/ as part of NPM and Bower, changing/removing it would be considered a breaking change (something to keep in mind for an eventual 3.0).

So, the question then is, do we need to keep dist/ or can we eliminate in favor of using qunit/ all the time? I tend to think we should be able to, though it may take some work. This would be particularly beneficial when working locally and attempting to npm link (or similar) into other packages.

@Krinkle
Copy link
Member

Krinkle commented Apr 5, 2017

@trentmwillis Sounds good.

I wasn't aware that we also use qunit/ for main in package.json for use through require() in Node.js. (I would've thought that we change main from dist to qunit in the release commit only, and not in master. If I understand correctly, this means it's currently broken in master during local development.)

@Krinkle Krinkle added this to the 3.0 milestone Aug 22, 2020
@Krinkle Krinkle added Status: Ready A "Meta" type issue that has reached consensus. and removed Type: Meta Seek input from maintainers and contributors. labels Aug 22, 2020
@Krinkle Krinkle removed this from the 3.0 release milestone Nov 7, 2020
@Krinkle Krinkle mentioned this issue Nov 7, 2020
14 tasks
@Krinkle Krinkle added this to the 2.x release milestone Nov 11, 2020
@Krinkle Krinkle changed the title Why dist/ and qunit/ folders? Phase out qunit/ folder in favour of dist/ Jan 1, 2021
@Krinkle Krinkle changed the title Phase out qunit/ folder in favour of dist/ Phase out dist/ folder in favour of qunit/ Jan 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Release help welcome Status: Ready A "Meta" type issue that has reached consensus.
Development

Successfully merging a pull request may close this issue.

4 participants