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

Build system #142

Merged
merged 2 commits into from
May 2, 2021
Merged

Build system #142

merged 2 commits into from
May 2, 2021

Conversation

hildjj
Copy link
Contributor

@hildjj hildjj commented May 1, 2021

This is a rework of #107. I simplified a few things, removed a few things that we're not using yet, and made sure that the web online, benchmark, and tests are using the new builds correctly. Web tests still use mocha for the moment, but getting Jest to work in the browser isn't trivial, and jest-lite didn't work out of the box for me.

"*.min.js"
"*.min.js",
"build",
"rollup.config.js" // in .eslintrc-modules.js
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we start using more modules, we can add '*.mjs' here, and use the second call to eslint to get those also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, mjs doesn't exist

- name: Test ${{ matrix.node-version }}
run: npm run test:bare
run: npm install && npm run build
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm build runs lint now.

@@ -0,0 +1,38 @@
{
"compilerOptions": {
"target": "es5", /* Specify ECMAScript target version: 'ES3' (default), 'ES5', 'ES2015', 'ES2016', 'ES2017', 'ES2018', 'ES2019', 'ES2020', or 'ESNEXT'. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this from es6 to es5 is what makes this a babel replacement step. It makes the es6 output nearly useless however, so I removed that for the moment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, i forgot about this, sorry

it'd be best to emit them from separate paths through compilation. es5 and es6 end up substantially different in size and speed due to the shims and shams involved, and the es6 downstreams can do a better job of tree shaking when unshimmed

@Mingun
Copy link
Member

Mingun commented May 2, 2021

As I already said in #107, I do not like the way how it implemented. Splitting that to small, isolated steps will be better.

Objections were @StoneCypher based on the fact that this is impossible (or very, very difficult).

Looking at the final list of modified files in this PR, it seems highly exaggerated to me.

There are, at least, many fixup commit, many of them just undoing changes from the previous commit(s). For example, commit, that adds coverage folder under the version control (why?) and commit, that removes it.

Deletion of the node 10.x and addition it back in the next commit. The addition commit itself does too many work:

  • replaces rf -rf with rimraf
  • fixes running some npm tasks on Windows (by direct invoking node instead of shebang magic)
  • changes npm scripts:
    • removes pretest step with parser regeneration
    • renames some steps
  • removes browserify (and, transitively, babelify)
  • adds typescript compiler + jest + rollup
    • even that work is done not completely, because the next commit does some fixes in it
    • another fixup for jest mixed up with the version autoupdating. That commit also deletes files, that was added in the previous jest commit

Some strange commits returning stuff, removed by the couple commits ago.

Yet another round of throwing back and forth

Clearly an experimental commit that shouldn't be in history.

Fixup commits, yet another fixups


I believe that upgrade can be made in more understandible way. If not decoupled into small pieces, than just in one commit that removes everything and another that adds the new infrastructure.

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

I think the best I can do at this point is squash-merge 107 then drop this on top. I think that gets us to a better place than we were, and we can move forward from there.

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

I'm going to follow the above plan in an hour or so unless anyone complains.

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

... except I can't squash merge 107 since the original repo is private now. I'm tempted to just squash this whole thing, commit it as me, and make sure @StoneCypher still gets credit in the comments and release notes.

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

I think I can add a Co-authored-by in the commit.

StoneCypher and others added 2 commits May 2, 2021 14:48
… and npm build

Adds Typescript, Rollup, Terser, and Jest.

Author:    John Haugeland <stonecypher@gmail.com>
@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

OK, I think I got all of the commits squashed, and attributed to the right people (probably off by a line or two while i dealt with conflicts). I'm waiting for the tests to pass, then will take another look through to see if everything looks right.

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

Fixes #100
Fixes #96
Fixes #94
Fixes #83
Fixes #75
Fixes #90
Fixes #77

@hildjj
Copy link
Contributor Author

hildjj commented May 2, 2021

Everything looks good, I'm pulling the trigger.

@hildjj hildjj merged commit 58ee201 into peggyjs:main May 2, 2021
@hildjj hildjj deleted the AlsoJest branch May 2, 2021 21:04
@Mingun
Copy link
Member

Mingun commented May 3, 2021

OK with that. If I had more time, I would have time to study the tools and would have tried to do what I described, but did not have time 🤷

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