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.

Sign up for GitHub

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

Jump to bottom

Internal Reorganization & Related Documentation #13

Merged
merged 10 commits into from
Nov 25, 2019
Merged

Internal Reorganization & Related Documentation #13

merged 10 commits into from
Nov 25, 2019

Conversation

agilgur5
Copy link
Owner

@agilgur5 agilgur5 commented Nov 24, 2019 •
edited
Loading

  • splits out the webpack and browserify configs into their own files instead of script arguments
  • reorganizes the files into directories so that app root isn't so cluttered
    • and so people don't get the wrong idea about what certain files are for, like mistakenly thinking the webpack and browserify configs are for building this library when they are only for testing this library
  • optimizes the test builds with noParse and externals
  • adds documentation for how a user may similarly apply bundler performance optimizations in their own builds, primarily around ammo
  • adds documentation describing the physijs modifications this library has made

@agilgur5
(env/refactor): split out webpack and browserify config
8ac13d2 
- split them into files instead of command-line args in package.json
  scripts
  - webpack has a config file ofc, but wanted to keep them consistent
    initially, so used command-line
    - decided to use the browserify API as its "config file"
      - this actually took a decent amount of research to figure out
        how to output to file (have to pipe to fs stream)
@agilgur5
(optim): don't parse ammo, keep three external
8021e1f 
- speeds up builds
- tests still pass, so I think it's okay to do, but this may be
  different from how developers use it so not necessarily ideal
  test-prod parity
  - though it may make sense to advocate for noParse as
    performance optimizations

- getting three to be properly require'd as an external with webpack
  meant I had to use libraryTarget for some reason...
  - otherwise it would expect it as a global
@agilgur5
(refactor): move test-utils/ test/utils/
e216692 
- going to move tests there too in order to declutter app root
@agilgur5
(refactor): move test files into test/
d1156a5 
- app root has a lot of files in it, this should help declutter a bit
@agilgur5
(refactor): move build configs into test/build-config/
feed6a3 
- having the build configs in app root made it kind of seem like they
  were being used to produce the library's export, but they're solely
  used for testing purposes
  - so moving them into test/ seems to make sense to me

- also to declutter app root similar to previous commits

- use process.cwd() in webpack config as __dirname is no longer app
  root, but process.cwd() still is as it's still called from
  package.json scripts
@agilgur5
(fix): resolve incorrect browserify noParse config
4a17ca8 
- upon further testing, ammo was still being parsed by browserify
  until now
  - had to time both webpack and browserify in order to tell it wasn't
    being parsed, as they don't error if the module isn't found
    - but they both compile significantly faster when it's not, so
      that's how I could tell I did it correctly (probably could've
      used verbose mode as well)
    - webpack was not parsing it, but had to double check

- browserify apparently only uses absolute paths, so require.resolve
  is the primary way of using noParse (would be great if this were
  better documented...)
  - while this is not strictly necessary for webpack, and it was
    working prior to this, changed the webpack config to use
    require.resolve as well for consistency
agilgur5
agilgur5 commented Nov 24, 2019
View reviewed changes
Copy link
Owner Author

@agilgur5 agilgur5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Content looks mostly good, just some small changes anc considerations!

  • Made a few spot-check fixes and commit message improvements in the first pass before officially reviewing.
  • Read through the markdown files on the actual GitHub branch to confirm how they look and read
  • Tests still pass on CI!

Might want to consider adding a publish commit here as well as throwing in a CI upgrade to Node 10 Active LTS... or do both separately?

physijs/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
physijs/README.md Outdated Show resolved Hide resolved
physijs/README.md Show resolved Hide resolved
physijs/README.md Show resolved Hide resolved
@agilgur5
(docs): add a README describing physijs changes
785978a 
- this makes it more explicit why it's been vendored in -- it has
  incompatible changes -- and makes more explicit what those changes
  are with documentation (instead of just commit messages)
  - it also explains why those changes were made, with lots of links
    as there's a decent bit of context around that
    - and this context is key to what made this library possible, so
      it's good to have finally documented it!

- also add the same worker links to the app root README for anyone
  who might want more info on those

- (pkg): modify package.json files config so that this new README
  isn't included in the package
@agilgur5 agilgur5 force-pushed the split-configs-reorg branch 4 times, most recently from 96172aa to 25af44b Compare November 24, 2019 08:33
@agilgur5
(docs): add a section on bundler performance optimizations
353b048 
- this is likely an area of concern for users and may even be a
  barrier to adoption for this library, so document it directly in
  the README to reach the greatest amount of people
  - some users may not even know these optimizations exist, so this
    should particularly help them in getting up to speed (pun intended)

- major downside of documenting this officially is that it must now be
  supported / maintained -- namely, this means that changing the path
  to ammo.js would now be considered a breaking change
  - but this library doesn't go through many changes and all that
    physijs code has been split into its own directory, so that's a
    trade-off I'm willing to make
@agilgur5 agilgur5 force-pushed the split-configs-reorg branch 2 times, most recently from 70c4b1b to fac587b Compare November 24, 2019 08:57
@agilgur5
(refactor/docs): split out bundler perf optimizations
2cd7696 
- made a lot of iterations on the bundler perf section just to really
  make it explicit that it's optional, but honestly the best way to
  do that is to just take it out of the README entirely
  - now its still linked to in the README, but not attracting a lot
    of attention to itself
  - downside of this is of course that less people who need it actually
    see it, but better than turning off users early with lots of config
    - README is tiny though, so it should still be easily noticeable
    - and hopefully the big blue hyperlinked heading makes it easily
      noticeable as well, so that hopefully prunes the downside quite
      a bit
agilgur5
agilgur5 commented Nov 24, 2019
View reviewed changes
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Ok, iterated a ton of times on getting the bundler perf section to look explicitly optional, and noticeable, but not attracting too much attention to itself or looking unwieldy to new users... think we've finally got a balance here by splitting it out into a separate file but giving it a big heading hyperlink in the README!

all the other changes look good too 👍

Notably, even on CI, the webpack build time improved by an order of magnitude, from ~3272ms to ~319ms. This isn't a big difference for our CI, which runs under a minute and this basically fits into the margin of error, but for users constantly building apps that use physijs-webpack, that's a huge difference, especially for slower machines. Good to see 🙂

@agilgur5 agilgur5 force-pushed the split-configs-reorg branch 2 times, most recently from b0f2eef to ae5d1ed Compare November 24, 2019 09:15
@agilgur5 agilgur5 mentioned this pull request Nov 25, 2019
Open
@agilgur5
(pub): v0.1.4 -- optimization docs & internal refactoring
7485a33 
no API surface changes
- the physijs code (physi.js, physijs_worker.js, and vendor/ammo.js)
  has all been moved from app root to physijs/
  - usage of the internal physijs code was not supported or documented
    before, but if you were somehow relying on that, it has now changed

docs improvements
- adds a Bundler Performance Optimizations doc that mainly details how
  one may use the `noParse` config on `ammo` to speed up build times
- adds a README to the physijs code to more explicitly describe what
  kinds of modifications were made to it to work for our use case
- only reports master branch's build status in app root README

internal improvements
- optimizes test bundle speed with the `noParse` option
- reorganizes code into directories to declutter app root
- splits out the test build config from package.json scripts into
  their own config files
- fixes changelog issues with the package.json repository field format
agilgur5
agilgur5 commented Nov 25, 2019
View reviewed changes
Copy link
Owner Author

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Ok, well adding a commit here to upgrade CI to Node v10 Active LTS somehow caused even more test errors, so removed that and just added a publish commit and that's all.

Since there's added docs here that rely on the new physijs/ directory, I definitely think this should be published as soon as it is merged to reduce any potential for confusion. Should be good to merge whenever!

With regard to the failing tests, will likely have to move the tests to cypress or a headless browser to get everything to work correctly.

@agilgur5 agilgur5 merged commit 713f6a9 into master Nov 25, 2019
@agilgur5 agilgur5 deleted the split-configs-reorg branch November 25, 2019 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add Bundler Perf section with tips?
1 participant
@agilgur5