-
-
Notifications
You must be signed in to change notification settings - Fork 78.9k
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 overhaul. #22045
Build system overhaul. #22045
Conversation
How best can I give feedback on this @bardiharborow? Do you have a checklist of remaining todos, or specific things that need testing? |
@mdo I've added a list of tasks and their review status. A local |
|
Pls consider using stylelint over scss lint as it offers far more control and flexibility in code control with plugin support for additional control |
@hanakin We'll be shipping that after this PR merges. It's just going to require a lot of config wrangling, and I'm juggling enough changes in this PR as it is. |
Preparing to ship this. I have every expectation that something will go wrong and I'll be thoroughly trouted for it, but anyway... |
package.json
Outdated
"js-lint": "eslint js/ && eslint --config js/tests/.eslintrc.json --env node build/", | ||
"js-lint-docs": "eslint --config js/tests/.eslintrc.json docs/assets/js/", | ||
"js-compile": "npm-run-all --parallel js-compile-*", | ||
"js-compile-bundle": "shx cat js/src/util.js 'js/src/!(util|popover).js' js/src/popover.js | shx sed 's/^(import|export).*//' | babel --filename js/src/bootstrap.js | node build/stamp.js > dist/js/bootstrap.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bardiharborow,
I have an error when this task is runned see :
I know my screenshot is in french, but it said :
'popover).js'' isn't known at an intern or extern command
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. I thought that shx was performing the glob expansion, but maybe bash is doing it, and that obviously won't work on Windows. I'll replace this with a static list for now.
not sure if it's something broken at my end, but: i fetched and rebased my fork on my machine, did a new [edit: ah, don't mind me...i guess the whole way of building is now different and i should build the local docs and read them...] |
They are just one task on our Gruntfile.js the other are in package.json And for example to run the |
so forgive me being thick, but...what's the equivalent now of the old |
It seems it's |
tried (every time the build process changes subtly i end up with not being able to do anything on my windows machine until i uninstall everything and then reinstall everything based on the docs...so if the relevant docs could also be updated to reflect anything that needs to be done, and in what order, that'd be super) [edit: rerunning |
Yes it seems they are several bugs on Windows (I'm on Windows too) a PR is open here #22477 we should report any issues here |
Awesome to see this merged! You're amazing @bardiharborow. Super happy to see things consolidated :). Only hiccup has been some Sauce failures with running the Sauce grunt task. See #22395 and #22496. Unsure what's going on there just yet. |
Would it make sense to have a
|
This PR switches every build task to a NPM script, except saucelabs (this is surprisingly non-trivial, and I'm pushing it to a later PR as this has taken way too long as it is). Closes #20332.
List of tasks that need to included. A ticked checkbox indicates that the task has been tested.
cp -r dist/* docs/dist/
)Push to GitHub(punting)Push to npm, etc.(punting)