Frontend refactor: move everything to imports/
#3231
Conversation
To get the remaining packages into imports, we need to hoist them up in the reverse order they are loaded. `blackrock-payments` is a leaf in that dependency tree; let's lift that one first. We move Blackrock's Stripe dependency into sandstorm's top-level package.json. We should be able to swap out Npm.require() for plain old imports. The two asset files are placed in private/, per the Meteor Guide.
|
To my knowledge, nobody is currently working on significant frontend work right now. I am not super versed in the JS world, but I feel the shell both should be the easiest place to iterate, and is the part of Sandstorm that I feel has been kept up with the least for a while. Ensuring working on it is more accessible to modern JS developers is a really worthy goal. Also, hi! |
|
It's exciting to see an old face jump back in. Generally positive about this change; I am in favor of explicit imports & exports and minimal import-time side effects. As @ocdtrekkie mentions I don't think anyone has any WIP stuff on the shell right now. The stuff I'm working on is on the bridge, @abliss has been working on CI stuff. Re: typescript, now that we've updated to meteor 1.8.2 I think we can just I'd like to keep the window of "don't touch anything" small if possible though. How long do you think this would take to get past that period? |
This is probably a good candidate for reworking later -- rethinking how we expose loginServices on client and server seems pretty reasonable given how little logic the two actually appear to share.
This appeared to have a dependency on a particular version of the content-type package from npm. We can sort that out separately. Additionally, this has a Meteor package dependency on fongandrew:find-and-modify. I pinned the version because I vaguely recall this particular library breaking in the past when we tried to update it and don't want to deal with that right now either.
|
This branch now covers "move all the code into The next steps will be approximately:
|
zarvox
changed the title
imports/
|
I'm hitting some test failures when I run the testsuite locally, so probably don't merge this quite yet unless you have reason to believe I haven't broken anything. Trying to figure out if they're just flakiness or true issues. Also, once upon a time we had tests that ran automatically against PRs? Do those still exist? |
|
Four tests currently fail in master. Bit of detail on #3208 comments. |
|
If #3208 gets merged, which it should, since it only fails tests that Kenton also fails when he builds on his machine, then we will have tests run automatically against PRs. The Jenkins server is long gone, but that PR gets tests running under GitHub Actions (which is free! woo!) |
Turns out we do in fact have a dependency between the two files in that package.
|
This PR is failing the same four tests locally, plus one more: Seems possible that importing a template with a |
Imported blaze templates don't appear to propagate <head> correctly, for whatever reason. So ignore it and accept that we have one template not in imports for <head> and <body>.
|
https://www.meteor.com/tutorials/blaze/templates is not particularly helpful here. It appears to be carefully sidestepping what appears to be the fact that importing a template that contains a |
|
Awesome work! This change will probably require more thorough manual testing than usual, to catch obscure breakages not covered by the tests. But, I think it makes sense to merge now to minimize conflicts with any other work, and then we'll just have to spend some time testing things before the next release. Also, it sounds like subsequent work will be more incremental and less likely to conflict with any concurrent changes, which is great. |
|
Master now looks like this for me:
Did you confirm this was working before merging? |
|
From sandstorm.log: |
|
False alarm; when fixing the merge conflicts on #3155, somehow I ended up with a |
|
I think this is because |
|
Aha, that makes sense. |
tl;dr move everything to the imports/ folder and take explicit responsibility for load order. Eventually, move toward a model where each file imports everything it depends on, and there are very few imports made strictly for their side-effects to the namespace.
Modern JS project structure and tooling has changed a lot in the past few years, and the layout of the sandstorm shell limits what tools we can readily make full use of. For instance, it would be very nice if we could rely on ESLint to tell us if we're making use of a global that we haven't imported or to eventually use TypeScript to tell us we're failing to handle a possible shape of data.
Meteor has moved to a model where they generally encourage folks to make use of
npmover Meteor packages, and Sandstorm already relies on many more npm packages than third-party Meteor packages. I believe the only Meteor packages that we use that are not maintained by either us or by Meteor themselves are theiron:routerpackage and thefourseven:scsspackage, the latter of which does not even impact JS. In the interest of simplifying the answer to the questions "where does code come from?", we can move our own packages intoimportsand prefer to use a single npmpackage.jsonto pull in dependencies.This will allow easier refactoring as all load order questions within Sandstorm become answerable in one place -- an explicit series of imports in a single server or client main file. We can over time reduce the number of imports-for-side-effects by restructuring code to only make use of things that it explicitly imports (and thus, that something else explicitly exports). Then, we can burn down the length of the import list in the client/server main files.
This pull request moves all JS code and HTML templates from
client/,server/,shared/,lib/, andpackages/to live in reasonable places underimports/. It sets up a single clientmain.jsand servermain.jsentry point for the client and server, which each explicitly import all the files that Meteor would previously have done in the same load order.